-
Notifications
You must be signed in to change notification settings - Fork 287
System.Diagnostics.Activity enumeration benchmarks #1457
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
Conversation
| { | ||
| int total = 0; | ||
|
|
||
| for (int i = 0; i < NumberOfActivities; i++) |
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.
for [](start = 12, length = 3)
do we really need to loop? Benchmark already repeat the method execution anyway and we can get he exact number for the enumeration.
I would suggest too to create multiple static activities with different tags count. we can have the perf numbers for short, medium and long tags count.
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.
Removed the looping. Added small/large tests.
| } | ||
|
|
||
| [Benchmark] | ||
| public void OTelHelperActivityTagObjects() |
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.
OTelHelperActivityTagObjects [](start = 20, length = 28)
could you please use the full word OpenTelemetry instead?
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.
Also, are we using OpenTelemetry because this how it call the Activity for enumeration? Does OTel create Enumerator as this test does? I am asking to know if it is worth it to complicate the test here. I am not pushing back though.
In reply to: 473440210 [](ancestors = 473440210)
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.
Removed all the OpenTelemetry stuff.
| } | ||
|
|
||
| // A helper class for enumerating over IEnumerable<TItem> without allocation if a struct enumerator is available. | ||
| private class Enumerator<TEnumerable, TItem, TState> |
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.
Enumerator [](start = 22, length = 10)
I am not really sure if it is good to test the internals with the reflection and code emit. what is really the goal here?
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.
These benchmarks were written for the other PR, really. I needed to make sure the struct enumerators worked for completely removing the allocation with the reflection mechanism, and also show there was an allocation reduction for the standard enumeration too. I opened this PR because I think @noahfalk said it would be useful. So my goal now is just to provide usefulness to future users of Activity 😄 I can remove the OpenTelemetry stuff, or I can close the PR completely if we don't think it will be that useful. LMK
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.
Adding perf tests for Activity is useful. I suggest removing all OTel stuff and keep the simple test cases for the enumerating tags, links...etc. Thanks a lot for all your provided help.
|
CC @adamsitnik if he has any comment. |
|
@CodeBlanch thanks for updating it. LGTM. I assume you ran it and the output is reasonable to you. right. @adamsitnik @DrewScoggins looks I don't have a permission to merge in this repo. could you please have a look and merge it if you don't have any comment? Thanks. |
|
@tarekgh Ya it's running fine. Here's the output:
|
|
LGTM |
Adding the benchmarks used on dotnet/runtime#40544
/cc @noahfalk @tarekgh