Reduce usage of unsafe constructs throughout codebase - #7426

Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged

Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom
GrabYourPitchforks:reduce_unsafe

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Mar 18, 2025

Copy link
Copy Markdown
Member

This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.

Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)

I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.

CopilotAI reviewed Mar 18, 2025
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)

src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207

  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov

codecovBot commented Mar 19, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.

Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).
Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tarekgh
tarekgh reviewed Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs Outdated
tarekgh
tarekgh reviewed Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
}
}
tmp = vector[i] - mean;
sum += tmp * tmp;

@tarekghtarekghMar 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp;

I assume we don't care about the overflow here, I know there is no change here from the old code behavior.

@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.

You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.

@tarekgh

tarekgh commented Mar 19, 2025

Copy link
Copy Markdown
Member

Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.

https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result

�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh approved these changes Mar 19, 2025
View reviewed changes

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@GrabYourPitchforks
GrabYourPitchforks merged commit 444573b into dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks deleted the reduce_unsafe branch March 20, 2025 15:41
@tarekgh

tarekgh commented Mar 24, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Reviewed post-hoc, LGTM

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Reduce usage of unsafe constructs throughout codebase - #7426
Merged
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Mar 20, 2025
Merged
Reduce usage of unsafe constructs throughout codebase#7426
GrabYourPitchforks merged 4 commits into
dotnet:mainfrom 
GrabYourPitchforks:reduce_unsafe
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Mar 18, 2025

Copy link


Copy Markdown

Member
This reduces the use of raw pointers and replaces them with safer constructs where possible. In some cases, MemoryMarshal.Read<T>, MemoryMarshal.Write<T>, and MemoryMarshal.AsBytes<T> are used.
Though these three methods are normally unsafe-equivalent APIs, they are guaranteed safe when T is a primitive integral or floating point type, char, or an enum thereof. (That is, it's guaranteed safe when T is byte, sbyte, short, ushort, char, int, uint, long, ulong, int128, uint128, nint, nuint, float, double, Half, or an enum of any of these.)
I've left code comments where things couldn't be made fully safe. I've also opted not to touch some code in VectorUtils.cs. I only touched code where the loop logic is simple enough for the JIT (even the older netfx JIT!) to always elide bounds checks. I didn't touch code paths where multiple buffers were being touched at once since the JIT doesn't yet properly elide bounds checks in those cases, and I didn't want to risk a possible perf regression.
CopilotAI
reviewed
Mar 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview
This PR reduces unsafe pointer usage and replaces it with safe MemoryMarshal and checked arithmetic operations. Key changes include replacing unsafe blocks with MemoryMarshal APIs in ToByteArrayExtensions.cs, refactoring stream reading methods in Stream.cs to use span-based operations, and removing unsafe code from VectorUtils.cs for vector operations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
FileDescription
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.csReplaces unsafe pointer manipulation with MemoryMarshal calls and adds checked arithmetic.
src/Microsoft.ML.Core/Utilities/Stream.csRefactors binary reading to use span-based operations via a new ReadBinaryDataIntoSpan method.
src/Microsoft.ML.FastTree/Utils/VectorUtils.csRemoves unsafe blocks from vector operations, consolidating loops into safe code.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs:207
  • The method name 'MutiplyInPlace' is misspelled and should be 'MultiplyInPlace'.
public static void MutiplyInPlace(double[] vector, double val)

@codecov
codecovBot
commented
Mar 19, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 57.14286% with 54 lines in your changes missing coverage. Please review.
Project coverage is 69.01%. Comparing base (adad40c) to head (4c46e4f).

Report is 2 commits behind head on main.
Files with missing linesPatch %Lines
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs46.77%33 Missing ⚠️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs26.92%19 Missing ⚠️
src/Microsoft.ML.Core/Utilities/Stream.cs94.73%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7426 +/- ##
==========================================
+ Coverage 68.97% 69.01% +0.03% 
==========================================
Files 1481 1481 Lines 273708 273386 -322 Branches 28285 28188 -97 ==========================================
- Hits 188789 188671 -118 + Misses 77525 77352 -173 + Partials 7394 7363 -31 
FlagCoverage Δ
Debug69.01% <57.14%> (+6.40%)⬆️
production63.31% <57.14%> (+0.70%)⬆️
test89.46% <ø> (∅)
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
src/Microsoft.ML.Core/Utilities/Stream.cs69.38% <94.73%> (+1.40%)⬆️
src/Microsoft.ML.FastTree/Utils/VectorUtils.cs9.95% <26.92%> (-1.55%)⬇️
...crosoft.ML.FastTree/Utils/ToByteArrayExtensions.cs35.56% <46.77%> (+1.64%)⬆️
... and 7 files with indirect coverage changes
🚀 New features to boost your workflow: 
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Core/Utilities/Stream.cs
Outdated

tarekgh
tarekgh
reviewed
Mar 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/Utils/VectorUtils.cs
 }
 }
 tmp = vector[i] - mean;
 sum += tmp * tmp;
@tarekghtarekghMar 19, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmp * tmp; 
I assume we don't care about the overflow here, I know there is no change here from the old code behavior.
@GrabYourPitchforksGrabYourPitchforksMar 19, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This will end up being double.Infinity if there's overflow.
You saw that I sprinkled some overflow checks throughout the code, but they're only in cases where we calculate array indices. I didn't add any overflow checks elsewhere, including in places where we calculate array values (as opposed to indices). That seemed like too risky a change to the core logic.
@tarekgh
tarekgh
commented
Mar 19, 2025

Copy link


Copy Markdown

Member
Seeing the following failure, I hope it is not related. I'll restart this test to see if it will pass or it is consistent failure.
https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-7426-merge-9e855dccaf3f484d86/Microsoft.ML.Core.Tests/1/console.91a4c382.log?helixlogtype=result
�[mStarting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression
Finished test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression with memory usage 192,921,600.00 and max memory usage 0.00
Starting test: Microsoft.ML.RunTests.TestEntryPoints.EntryPointPercentileThreshold
�[31;1m Microsoft.ML.RunTests.TestEntryPoints.EntryPointEvaluateRegression [FAIL]
�[m�[37m System.ArgumentOutOfRangeException : If specified, must be non-negative. (Parameter 'valuesCount')
�[m�[30;1m Stack Trace:
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.DataView/VBuffer.cs(427,0): at Microsoft.ML.Data.VBuffer`1.GetEditor(Int32 newLogicalLength, Nullable`1 valuesCount, Nullable`1 maxValuesCapacity, Boolean keepOldOnResize, Boolean requireIndicesOnDense)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(1398,0): at Microsoft.ML.Data.CacheDataView.ColumnCache.ImplVec`1.Fetch(Int32 idx, VBuffer`1& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/DataView/CacheDataView.cs(496,0): at Microsoft.ML.Data.CacheDataView.RowCursor`1.<>c__DisplayClass8_0`1.<CreateGetterDelegateCore>b__0(TValue& value)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerUtils.cs(555,0): at Microsoft.ML.Trainers.TrainingCursorBase.MoveNext()
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaBinary.cs(385,0): at Microsoft.ML.Trainers.SdcaTrainerBase`3.TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, Int32 weightSetCount)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/StochasticTrainerBase.cs(43,0): at Microsoft.ML.Trainers.StochasticTrainerBase`2.TrainModelCore(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(100,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer<Microsoft.ML.IPredictor>.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs(168,0): at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Microsoft.ML.ITrainer.Train(TrainContext context)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(280,0): at Microsoft.ML.Data.TrainUtils.TrainCore(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, RoleMappedData validData, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples, Nullable`1 cacheData, IPredictor inputPredictor, RoleMappedData testData)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/Commands/TrainCommand.cs(249,0): at Microsoft.ML.Data.TrainUtils.Train(IHostEnvironment env, IChannel ch, RoleMappedData data, ITrainer trainer, IComponentFactory`1 calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.Data/EntryPoints/InputBase.cs(119,0): at Microsoft.ML.EntryPoints.TrainerEntryPointsUtils.Train[TArg,TOut](IHost host, TArg input, Func`1 createTrainer, Func`1 getLabel, Func`1 getWeight, Func`1 getGroup, Func`1 getName, Func`1 getCustom, ICalibratorTrainerFactory calibrator, Int32 maxCalibrationExamples)
�[m�[37m /Users/runner/work/1/s/src/Microsoft.ML.StandardTrainers/Standard/SdcaRegression.cs(221,0): at Microsoft.ML.Trainers.Sdca.TrainRegression(IHostEnvironment env, Options input)
�[m�[37m at InvokeStub_Sdca.TrainRegression(Object, Span`1)
�[m�[37m at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
�[m�[30;1m Output:
�[m�[37m Automatically adding a MinMax normalization transform, use 'norm=Warn' or 'norm=No' to turn this behavior off.
�[m�[37m �[m�[37m �[m�[37m Using 2 threads to train.
�[m�[37m �[m�[37m �[m�[37m Automatically choosing a check frequency of 2.

tarekgh
tarekgh
approved these changes
Mar 19, 2025
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@GrabYourPitchforks
GrabYourPitchforks
merged commit 444573b
into
dotnet:mainMar 20, 2025
@GrabYourPitchforks
GrabYourPitchforks
deleted the

reduce_unsafe

branch
March 20, 2025 15:41
@tarekgh
tarekgh
commented
Mar 24, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Reviewed post-hoc, LGTM
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Apr 24, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tarekghtarekghtarekgh approved these changes
@michaelgsharpmichaelgsharpAwaiting requested review from michaelgsharp

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

reduce-unsafe

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@tarekgh