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.
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
LogEmitter System.Diagnostics.Tracing extension project #3305
LogEmitter System.Diagnostics.Tracing extension project #3305
Changes from 23 commits
6d2f153
101ce19
7eb0006
20479f5
c64ca11
53a6854
fd5a87e
035823b
80cbdbb
744c399
df93cf5
1ba8afe
9a0ee32
888eb54
558e8d2
4d76b87
617d88d
ca7e319
6fae76d
a92e78e
5e33474
6b15b3b
2114dca
0d34533
bb4293c
380f139
87597f8
345e1a2
ea63af4
a12d432
3b35268
57775a5
9ee18df
f347e51
5537c73
e0c2347
dd80cc9
4f6b9e0
1dcd193
226facf
d3c5443
31f5a97
cb69e49
8693d93
b71006f
6269015
7647145
4f498af
55537b5
d1a4b3b
ffda1ce
12c92ad
cf69b8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just confirming my own understanding of the improvements this PR brings... so this is more performant because accessing properties of the struct results in a
call
vs.callvirt
, right?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.
This particular change is more to prevent a perf regression! This method used to work something like...
(Because
LogRecord
is sealed it should already be acall
instead ofcallvirt
.)Now due to the pooling changes it has to do...
Extra call!
Using
GetDataRef()
it goes back to a single call like...Just an optimization to cut out the extra hops.
True perf really comes down to how the JIT decides to inline things. Here's a sharplab if you want to mess with it.
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.
So what are the real perf gains on this PR?
I will attempt to answer this 😄
Our log logic was like this:
And is now like this:
Should be slower, right?
Turns out, is only slightly slower (when hitting the
[ThreadStatic]
):But we eliminated all the memory pressure. I think this is why the stress test is performing better with the pool. Only a few more CPU cycles to use the pool but the GC is asleep freeing up the CPU to process more logs. This is a largely a guess but kind of makes sense?
Now that benchmark is interesting. How is it introducing the pool logic made it only slightly slower than just calling ctor?
Check out this benchmark:
Number of fields on the class impacts the ctor time.
LogRecord
has a lot of fields so that gives us some wiggle room to execute our pool logic before just calling the ctor would be faster. Interesting, eh?