-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Improve Benchmark Accuracy #2336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
tests/BenchmarkDotNet.IntegrationTests.ManualRunning/ExpectedBenchmarkResultsTests.cs
Outdated
Show resolved
Hide resolved
48266cd
to
4abbb1f
Compare
213549a
to
7080e63
Compare
3e4f122
to
e3e774b
Compare
Unfortunately, I no longer have an Intel CPU to test this, but I think the new results from this PR should address the issues you had in #2334 @AndreyAkinshin (results look good on my AMD 9800X3D CPU in .Net 8). |
e3e774b
to
f1a89ed
Compare
@AndyAyersMS Adam mentioned you are the expert I should tag to get your opinion on things like this. Do you have any thoughts or concerns about this approach? |
I am not that familiar with how robust the weaving process is... maybe it would be good (if possible) to validate it on say the dotnet/performance suite of 5000ish benchmarks. I guess it also means there is no simple faithful source representation for the benchmark assembly, which might trip up some users that keep the sources around and use those for investigations? What happens if you just compile and run those sources? Do you have examples showing the impact on reported results?
Can you explain this a bit more? The overhead is now some "fixed" workload measurement regardless of benchmark method signature? |
Overhead always returns `void`.
…thods. Count down loops instead of count up. Added IntroSmokeStringBuilder. Added more return type test cases.
Reverted loop methods back to `AggressiveOptimization`. Added `NoInlining` to `__Overhead` to match weaved benchmark method. Updated ExpectedBenchmarkResultsTests.
Update Weaver.
I can give it a try.
Could you elaborate? The weaver just adds
What do you mean? If the source project has a reference to
With the benchmarks from #1133, virtually no impact on my Ryzen 9800X3D, slightly more accurate on Apple M3 (a CPU architecture which BDN seems to have overhead measurement issues for nano benchmarks). It seems to mostly impact Intel CPUs and older (pre-Zen) AMD CPUs (see results I obtained previously in #2334. Unfortunately I no longer have those old CPUs to test again). Apple M3 results
master:
PR:
As for returning a value (like the issue #2305), I'm not sure if my 9800X3D is the best representative CPU compared to what dotnet/performance runs on, but I got these results: [Benchmark] public Vector3 Vector3() => default; master:
PR:
It's a fix for #2305. Not quite, it still passes all the same arguments, only the return type is fixed to |
@AndyAyersMS Ignoring net462 (dotnet/performance#4869), I tried build with these changes, and I got this error.
I can't find anything about |
@LoopedBard3 any ideas? Android support? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A concise summary:
- Simplifies benchmark type code generation by removing legacy helpers, unifying loop IL generation, and using direct stack operations.
- Streamlines declarations providers to just Sync/Async variants, dropping multiple specialized classes.
- Introduces a Weaver task (
WeaveAssemblyTask
) inBenchmarkDotNet.Weaver
to automatically applyNoInlining
to all[Benchmark]
methods.
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/BenchmarkDotNet/Helpers/Reflection.Emit/MethodBuilderExtensions.cs | Removed unused GetParameterTypes extension and System using. |
src/BenchmarkDotNet/Helpers/Reflection.Emit/IlGeneratorStatementExtensions.cs | Refactored loop emission from index-based to decrement-based (EmitLoopBeginFromArgToZero / EmitLoopEndFromArgToZero ). |
src/BenchmarkDotNet/Helpers/Reflection.Emit/IlGeneratorEmitOpExtensions.cs | Replaced large indirect store switch with EmitStarg helper. |
src/BenchmarkDotNet/Helpers/Reflection.Emit/IlGeneratorDefaultValueExtensions.cs | Deleted obsolete default-value IL emission helpers. |
src/BenchmarkDotNet/Helpers/Reflection.Emit/IlGeneratorCallExtensions.cs | Removed unused DeclareOptionalLocalForInstanceCall . |
src/BenchmarkDotNet/Engines/Consumer.cs | Stripped out unused SupportedTypes and helper methods. |
src/BenchmarkDotNet/Code/DeclarationsProvider.cs | Collapsed multiple provider subclasses into SyncDeclarationsProvider and AsyncDeclarationsProvider . |
src/BenchmarkDotNet/Code/CodeGenerator.cs | Removed conditional compilation and wiring to old providers; updated provider selection logic. |
src/BenchmarkDotNet.Weaver/src/WeaveAssemblyTask.cs | Added WeaveAssemblyTask MSBuild task and custom resolver to apply NoInlining . |
Comments suppressed due to low confidence (3)
src/BenchmarkDotNet/Helpers/Reflection.Emit/IlGeneratorStatementExtensions.cs:123
- [nitpick] The inline IL comment references
IL_0002
, which no longer matches the updatedloopStartLabel
. Update or remove this outdated offset comment to avoid confusion.
// IL_0011: bge.s IL_0002
src/BenchmarkDotNet/Code/DeclarationsProvider.cs:51
- SyncDeclarationsProvider does not implement the abstract members
ReturnsDefinition
,OverheadMethodReturnType
, andOverheadImplementation
fromDeclarationsProvider
, which will cause compilation errors.
internal class SyncDeclarationsProvider : DeclarationsProvider
src/BenchmarkDotNet/Code/DeclarationsProvider.cs:56
- AsyncDeclarationsProvider is missing overrides for the abstract properties
ReturnsDefinition
andOverheadImplementation
fromDeclarationsProvider
, leading to incomplete implementation.
internal class AsyncDeclarationsProvider : DeclarationsProvider
Fixes #1133
Fixes #2305
Fixes #1802
Fixes #2530
void
for a fair baseline for all benchmark return types.Consumer
or aNoInlining
method.MethodImplOptions.NoInlining
to benchmark methods.