-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
avoid byte[] allocations when calculating cluster slot #2110
Conversation
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.
Looks great! One question bout #if defs I wanted to check on intent - thoughts on simplifying? Works as-is, but can simplify I think given one's never hit.
src/StackExchange.Redis/RedisKey.cs
Outdated
case string s: | ||
if (s.Length != 0) | ||
{ | ||
#if NETCOREAPP3_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER |
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.
Since we only target netstandard2.0
, simplify to #if NETCOREAPP
?
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.
Noted; I assume NETCOREAPP
includes 5.0+?
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.
Aye, it's not netstandard but all .NET Core onwards including 5/6/7, etc.
1. remove RedisKey.CompositeEquals - prefer CopyTo 2. use [SkipLocalsInit]
2. fix broken RedisKey.GetHashCode() !!!
@NickCraver see updates: also fixed equality etc operations, and removed a bunch of ugly code; also - it turns out
With the revisions: Equals, ==, !=, GetHashCode all behave correctly now. |
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.
Looking great - I went through all existing stackalloc
usages as well to double check we're ensuring bounds usage and agree it's good. As an aside: I noticed in there that I think VectorSafeIndexOfCRLF
can use some of the new compiler optimizations instead of creating the span each time perhaps?
Nice work!
@mgravell housekeeping question: we now have |
Yeah, I actually started hacking that, but: I'm not 100% sure what it compiles to on netfx, and sharplab can't help me. I'll dig out a desktop IL decompiler tomorrow, and check that it is close enough to what we expect. |
There is a subtle
byte[]
alloc in the cluster slot usage, which will be allocating a lot ofbyte[]
for cluster; let's... not do that[SkipLocalsInit]
? improves a few scenarios, but in particularstackalloc
byte[]
operator to useCopyTo