-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Speed up multiple attributes overwrite detection. Fixes #24467 #24561
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
Speed up multiple attributes overwrite detection. Fixes #24467 #24561
Conversation
deb9eae
to
98aac4c
Compare
98aac4c
to
dd42509
Compare
for (var i = last; i >= first; i--) | ||
{ | ||
ref var frame = ref buffer[i]; | ||
Debug.Assert(frame.FrameTypeField == RenderTreeFrameType.Attribute, $"Frame type is {frame.FrameTypeField} at {i}"); | ||
|
||
if (!seenAttributeNames.TryGetValue(frame.AttributeNameField, out var index)) | ||
if (!seenAttributeNames.TryAdd(frame.AttributeNameField, 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.
👍
// proportion of attribute names. | ||
return unchecked( | ||
char.ToLowerInvariant(key[keyLength - 1]) | ||
+ 31 * char.ToLowerInvariant(key[keyLength / 2]) |
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.
Can we assign the coefficients here to constants?
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.
Generally I'd prefer not to add the extra abstraction unless it's a case where we think there are multiple uses that need to be kept in sync. Let me know if you want to discuss it - maybe there are other reasons I'm missing!
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.
I posted the feedback in the interest of readability. I totally hear you about it not being technically necessary. Maybe the middleware group is an inline comment (e.g. 42 /* LIFE_THE_UNIVERSE_AND_EVERYTHING */
)
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.
Nice change overall! Nice to see a few lines of code bring about a nice perf boost.
Details in #24467
I spent ages on this because there were so many ways to approach it. The awkward thing is I can actually make this quite noticeably faster still than the code in this PR, by implementing:
System.Collections.Generic.Dictionary<T,U>
, but a different implementation altogether). This gives about another 2.5% gain on the benchmark on top of this PR.char.ToLowerInvariant
that special-cases ASCII, but falls back onchar.ToLowerInvariant
for other inputs. This gives another 1% gain.However after some wrangling I've decided not to do either of those. It's because even if my implementations are faster on the benchmark scenarios today, it's possible that:
System.Collections.Generic.Dictionary
in all extreme cases is well understood, but we can't say the same for new alternatives.char.ToLowerInvariant
can be expected to get faster, whereas custom implementations won't.So, the code as-is in this PR improves intensive attribute splatting scenarios by around 5%. Not just the attribute-splatting part of the process, but the timings of the entire scenario that involves intensive attribute splatting. That's pretty worthwhile even if we forgo the extra ~3.5% we could get with a custom dictionary and char-normalizer.