-
Notifications
You must be signed in to change notification settings - Fork 773
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
Add benchmark for SuppressInstrumentation #1049
Add benchmark for SuppressInstrumentation #1049
Conversation
{ | ||
BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args); |
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.
Changed this so you can run like:
dotnet run --configuration Release --filter SuppressInstrumentationBenchmarks
or
dotnet run --configuration Release --filter TraceBenchmarks
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.
@alanwest FYI the benchmark project in /test/ is already set up for that. We should combine these two benchmark projects at some point.
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.
Yep noticed that, that's where I copied the pattern. Put these next to @reyang's other benchmarks for now since they are similar, but we can discuss a better place once we're through with analysis.
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.
Ping back from #1039 (comment).
Codecov Report
@@ Coverage Diff @@
## master #1049 +/- ##
==========================================
+ Coverage 75.83% 75.85% +0.01%
==========================================
Files 225 225
Lines 6225 6225
==========================================
+ Hits 4721 4722 +1
+ Misses 1504 1503 -1
|
Looks like the memory allocation increased from 656B to 744B. We probably need to analyze where the tax is coming from using https://docs.microsoft.com/visualstudio/profiling/dotnet-alloc-tool. FYI @cijothomas @rajkumar-rangaraj @ThomsonTan in case you might be interested. |
@reyang I will investigate an increase in allocation. |
So I'm fairly certain the increase is simply the AsyncLocal allocation. Ran a simple console app: public static void Main(string[] args)
{
using var sourceWithSuppressInstrumentation = new ActivitySource("Benchmark.SuppressInstrumentation");
Activity.DefaultIdFormat = ActivityIdFormat.W3C;
ActivitySource.AddActivityListener(new ActivityListener
{
ActivityStarted = null,
ActivityStopped = null,
ShouldListenTo = (activitySource) => activitySource.Name == sourceWithSuppressInstrumentation.Name,
GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => { return Sdk.SuppressInstrumentation ? ActivityDataRequest.None : ActivityDataRequest.AllDataAndRecorded; },
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => { return Sdk.SuppressInstrumentation ? ActivityDataRequest.None : ActivityDataRequest.AllDataAndRecorded; },
});
for (var i = 0; i < 5000; ++i)
{
using (var activity = sourceWithSuppressInstrumentation.StartActivity("Benchmark"))
{
// this activity will be created and feed into an ActivityListener that simply drops everything on the floor
}
}
} The allocations per iteration for |
I left a comment declaring no more increase in allocations after cherry-picking #1067 into this PR. I lied 😬. Comment deleted. Here are the real results after cherry-picking:
SuppressInstrumentationFalse case is better but the AllDataAndRecordedListener2 case is worse. |
eff2abb
to
f3e3c3a
Compare
@reyang Here are the latest results. My interpretation is that the check (i.e., My question now is that since the "check" seems free of additional allocations do we want to move forward with #1079 or instead explore the "alternatives" I've referenced in the description of that PR?
|
I think if there is no additional memory allocation cost, and the extra CPU cost is less than 3%, it makes good sense to add it to the hotpath. |
When
Memory allocation is the same. CPU is actually slightly lower w/ the check. Seems like a bit of a wash. When
Memory allocation is naturally different between these cases. SuppressInstrumentationTrue allocates to AsyncLocal. Not a surprise that CPU is different here too. The differences look dramatic, though |
Related to #1079 and #1098.
@reyang