avoid byte[] allocations when calculating cluster slot #2110
Conversation
NickCraver
left a comment
NickCraver
left a comment
There was a problem hiding this comment.
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.
Since we only target netstandard2.0, simplify to #if NETCOREAPP?
There was a problem hiding this comment.
Noted; I assume NETCOREAPP includes 5.0+?
There was a problem hiding this comment.
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. |
NickCraver
left a comment
NickCraver
left a comment
There was a problem hiding this comment.
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 particularstackallocbyte[]operator to useCopyTo