avoid byte[] allocations when calculating cluster slot by mgravell · Pull Request #2110 · StackExchange/StackExchange.Redis (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
There is a subtle byte[] alloc in the cluster slot usage, which will be allocating a lot of byte[] for cluster; let's... not do that
- open question: happy for me to add
[SkipLocalsInit]? improves a few scenarios, but in particularstackalloc - refactor existing
byte[]operator to useCopyTo - check for other related scenarios - equality maybe?
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.
| case string s: |
|---|
| if (s.Length != 0) |
| { |
| #if NETCOREAPP3_1_OR_GREATER | |
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?
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+?
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.
- remove RedisKey.CompositeEquals - prefer CopyTo
- use [SkipLocalsInit]
- fix broken RedisKey.GetHashCode() !!!
@NickCraver see updates: also fixed equality etc operations, and removed a bunch of ugly code; also - it turns out RedisKey.GetHashCode() has always been a bit... broken - it didn't work correctly when doing either of:
- a string compared (hash-code) to the utf-8 bytes of the same string
- a prefix/suffix pair compared (hash-code) to the combined single value
With the revisions: Equals, ==, !=, GetHashCode all behave correctly now.
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 AssemblyInfoHack.cs (which I can move to .csproj), .Hacks.cs, NullableHacks.cs, and SkipLocalsInit.cs - how about I make a follow-up minimizing/consolidating some of that info a folder or some such after this is in?
VectorSafeIndexOfCRLF
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.
