Fix warnings native#8534
Merged
Merged
Conversation
NachoEchevarria
added
the
area:builds
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8534) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (75ms) : 70, 80
master - mean (74ms) : 71, 78
section Bailout
This PR (8534) - mean (78ms) : 75, 81
master - mean (78ms) : 74, 83
section CallTarget+Inlining+NGEN
This PR (8534) - mean (1,078ms) : 1032, 1125
master - mean (1,071ms) : 1029, 1113
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (117ms) : 110, 124
master - mean (117ms) : 110, 123
section Bailout
This PR (8534) - mean (115ms) : 113, 117
master - mean (116ms) : 111, 120
section CallTarget+Inlining+NGEN
This PR (8534) - mean (781ms) : 759, 803
master - mean (781ms) : 754, 807
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (105ms) : 100, 110
master - mean (101ms) : 97, 106
section Bailout
This PR (8534) - mean (102ms) : 99, 105
master - mean (102ms) : 99, 105
section CallTarget+Inlining+NGEN
This PR (8534) - mean (941ms) : 911, 971
master - mean (937ms) : 897, 977
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (100ms) : 97, 103
master - mean (103ms) : 97, 108
section Bailout
This PR (8534) - mean (105ms) : 99, 110
master - mean (101ms) : 98, 104
section CallTarget+Inlining+NGEN
This PR (8534) - mean (829ms) : 785, 874
master - mean (821ms) : 790, 852
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (201ms) : 193, 209
master - mean (202ms) : 194, 211
section Bailout
This PR (8534) - mean (205ms) : 197, 212
master - mean (206ms) : 199, 213
section CallTarget+Inlining+NGEN
This PR (8534) - mean (1,187ms) : 1135, 1240
master - mean (1,196ms) : 1133, 1258
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (289ms) : 275, 302
master - mean (287ms) : 276, 298
section Bailout
This PR (8534) - mean (289ms) : 277, 301
master - mean (288ms) : 279, 298
section CallTarget+Inlining+NGEN
This PR (8534) - mean (953ms) : 918, 989
master - mean (949ms) : 923, 975
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (281ms) : 272, 291
master - mean (281ms) : 272, 290
section Bailout
This PR (8534) - mean (284ms) : 273, 294
master - mean (283ms) : 272, 295
section CallTarget+Inlining+NGEN
This PR (8534) - mean (1,157ms) : 1109, 1205
master - mean (1,151ms) : 1109, 1193
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8534) - mean (282ms) : 269, 295
master - mean (280ms) : 268, 292
section Bailout
This PR (8534) - mean (283ms) : 271, 294
master - mean (281ms) : 272, 290
section CallTarget+Inlining+NGEN
This PR (8534) - mean (1,041ms) : 984, 1098
master - mean (1,041ms) : 1001, 1081
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-04-29 10:25:51 Comparing candidate commit 18261be in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 59 known flaky benchmarks, 28 flaky benchmarks without significant changes.
|
NachoEchevarria
marked this pull request as ready for review
April 28, 2026 15:00
zacharycmontoya
approved these changes
Apr 28, 2026
andrewlock
approved these changes
Apr 29, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Thanks! just a couple of questions
| sb.AppendLine(""" | ||
| }; | ||
| return trace::profiler->RegisterIastAspects((WCHAR**) callSites.data(), callSites.size(), enabledCategories, platform); | ||
| return trace::profiler->RegisterIastAspects((WCHAR**) callSites.data(), static_cast<int>(callSites.size()), enabledCategories, platform); |
Member
There was a problem hiding this comment.
Again, this is technically casting a 64-bit type to a 32-bit type - should we instead change the target method to accept a size_t instead of int, seeing as that's what it "should" be? I think that's a safe change to make, right, because these aren't "public" method calls per-se
Collaborator
Author
There was a problem hiding this comment.
Yes, It would be cleaner, thanks. Done!
andrewlock
reviewed
Apr 29, 2026
Comment on lines
+179
to
+182
| // Managed callers always pass an array length, so this should never trip; assert captures | ||
| // the assumption before promoting to the size_t parameter. | ||
| assert(aspectsLength >= 0); | ||
| return trace::profiler->RegisterIastAspects(aspects, static_cast<size_t>(aspectsLength)); |
Member
There was a problem hiding this comment.
Wouldn't it be better to make the outer RegisterIastAspects take a size_t here, too? 🤔
Collaborator
Author
There was a problem hiding this comment.
Yes, I was thinking about that, but that would complicate things a little bit beyond the scope of this PR because it would affect the interface between managed and native. We'd need to update the four [DllImport] declarations in NativeMethods.cs plus the RegisterIastAspects / RegisterCallTargetDefinitions C# wrappers.
Also, size_t is platform-sized (32-bit on x86, 64-bit on x64), and nuint/UIntPtr is the only managed type that matches it on both. nuint is C# 9+, that could be another issue. Also, the [MarshalAs] annotation expects the referenced parameter to be a primitive integer type — int, uint, long, ulong, short, etc. The .NET runtime documentation lists those explicitly. Whether it accepts nuint is fuzzy. That would be kind of a risky change without real benefit.
Member
There was a problem hiding this comment.
Yeah, that's fair. In which case, we should probably just drop the We could consider switching to assert (crashing would definitely be bad!).uint in the future I guess, so at least it's unsigned 🤷♂️
andrewlock
reviewed
Apr 29, 2026
e-n-0
approved these changes
Apr 29, 2026
dudikeleti
approved these changes
Apr 29, 2026
Collaborator
Author
|
Thanks for the feedback and reviews! |
NachoEchevarria
added a commit
that referenced
this pull request
May 7, 2026
## Summary of changes Part 3 of the warning-reduction series. ~25 warning sources eliminated across sample/regression projects and the runner tool. Previous PRs: #8534 #8519 ## Reason for change CI emits ~17k warnings; chipping away at the batch surfaced after parts 1-2 merged. ## Implementation details - **Deleted redundant `PackageReference`s** (NU1510 — flagged framework-provided on the target TFM and not needed): `Microsoft.AspNetCore.Mvc.RazorPages`, `Microsoft.Extensions.Logging.Debug`, `System.Runtime.InteropServices.RuntimeInformation`, `System.Data.Common`, `System.Net.Http 4.3.4`, `System.IO.FileSystem`, `System.Net.NameResolution`. - **Targeted `<NoWarn>` (with comment explaining why the reference is load-bearing)**: - NU1510 for `System.Memory` / `System.Net.Http` on net4x targets — Probes, CallTargetNativeTest, VersionConflict.{1x,2x}, DuplicateTypeProxy. - MSTEST0001/0044/0057 + NU1701 — MSTestTests, MSTestTests2, MSTestTestsRetries. - NETSDK1201 — ManualInstrumentation; ASP0014 — Ocelot.DistributedTracing. - IL2104 — `Datadog.Trace.Tools.Runner` (third-party assemblies already rooted via `<TrimmerRootAssembly>`). - **Nullability fixes**: `SampleHelpers.TrySetServiceName` parameter widened to `string?` with an early-return guard so we don't reflection-invoke the setter with null; redundant `evaluation?.Error` removed in `Samples.FeatureFlags/Evaluator.cs` (was de-narrowing the flow state and tripping CS8602 on net48); `Samples.GrpcDotNet/Worker.cs` `[CallerMemberName]` default fixed. ## Test coverage No behavioral changes. CI verifies the warning count drops without breaking compilation. ## Other details --------- Co-authored-by: Andrew Lock <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Eliminates the 32 native C++ build warnings (15 unique source positions) emitted by the Windows native build.
Reason for change
Follow-up to #8519 (sample warnings). Native was the next cluster where every warning is a real fix rather than structural noise.
Implementation details
<):auto x = 0→size_t x = 0in 5 loops inrejit_handler.cppandint i = 2→size_t i = 2in sharedcor_profiler.cpp. Loop vars only used as indices.size_t/ptrdiff_tnarrowing intoULONG/DWORD/intAPI params): explicitstatic_castat the call sites incalltarget_tokens.cpp,fault_tolerant_rewriter.cpp,process_helper.cpp,tracer_tokens.cpp, and the two generated.g.cppfiles. All inputs bounded well below the cast range. The corresponding generator templates intracer/build/_build/CodeGenerators/are updated alongside the generated outputs.SignatureBuilder::EnsureBufferSpaceparamint→size_tto match the rest of the class. Also clears a latent signed/unsigned mix in_offset + size >= _length.fault_tolerant_rewriter.cpp:86,97:std::string(wstr.begin(), wstr.end())→shared::ToString(wstr)(the C4244 trigger inside<xstring>). Identical behavior for ASCII generic-type names; UTF-8-correct for non-ASCII where the old form silently truncated.Test coverage
Other details