-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
More EventListener overhead reductions #52092
Conversation
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti Issue DetailsThis PR builds on top of #51822, focusing entirely on I special-cased Opening this PR to get some CI coverage as I added debug asserts around the user-supplied
I'd assume this is about as far as we can get with the current API shape (args+payload+boxing+strings).
|
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
TypeCode typeCode = Type.GetTypeCode(dataType); | ||
int size = data->Size; | ||
|
||
if (size == 4) |
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.
Wouldn't be a simple switch over typeCode instead of multiple levels of nested ifs faster?
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.
They don't always match up here.
For enums, EventSource treats them as int32
, but Type.GetTypeCode(dataType)
will resolve the underlying type.
That is why the [SByte, Int32]
range is being checked under size == 4
(and read as an int*
), but then also checked outside the size == 4
case.
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.
Unless you'd like to optimize for non-int enums it still seems to me that the switch would be better option
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.
In that case you would have duplication like
case TypeCode.Byte:
if (dataType.IsEnum)
{
Debug.Assert(size == 4);
return *(int*)dataPointer;
}
else
{
Debug.Assert(size == 1);
return *(byte*)dataPointer;
}
case TypeCode.SByte:
if (dataType.IsEnum)
{
Debug.Assert(size == 4);
return *(int*)dataPointer;
}
else
{
Debug.Assert(size == 1);
return *(sbyte*)dataPointer;
}
// ...
For byte, sbyte, short, ushort, int.
I can measure, but I doubt changing to a switch would be faster here.
Updated the benchmarks above with the latest changes |
// null in the string. | ||
#if DEBUG | ||
Debug.Assert(data->Size % 2 == 0, "String size should be even"); | ||
for (int i = 0; i < data->Size / 2 - 1; i++) Debug.Assert(*((char*)dataPointer + i) != 0, "String may not contain null chars"); |
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.
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.
Yup. There's currently nothing stopping users from putting a string with embedded nulls into EventSource
.
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.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
else | ||
{ | ||
for (int i = 0; i < args.Length; i++, data++) | ||
{ |
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.
There might be a little more perf to gain if the for loop is inside the method rather than outside of it. I'm guessing DecodeObject() isn't being inlined right now and making the loop body opaque inside the method doesn't give the JIT as much visbility to optimize. There is probably also a tiny overhead in repeated execution of the DecodeObject prolog/epilog.
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 DecodeObject
to DecodeObjects
. Control flow is a bit messier (yay for goto), but it does give us a few extra %.
Roughly ~10% improvement for WriteHttpRequestStart
.
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.
Oh nice, ~10% is even more than I would have guessed : )
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.
Thanks @MihaZupan!
|
Failure unrelated: #51819 |
This PR builds on top of #51822, focusing entirely on
DecodeObject
speed.I special-cased
AllParametersAreString
andAllParametersAreInt32
(as that seems to be by far most common).Make use of
Type.GetTypeCode
inDecodeObject
to speed up all the other cases.Opening this PR to get some CI coverage as I added debug asserts around the user-supplied
EventData.Size
.Benchmarks.cs
I'd assume this is about as far as we can get with the current API shape (args+payload+boxing+strings).