Home Original page

avoid byte[] allocations when calculating cluster slot by mgravell · Pull Request #2110 · StackExchange/StackExchange.Redis

@mgravell

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 particular stackalloc
  • refactor existing byte[] operator to use CopyTo
  • check for other related scenarios - equality maybe?

@mgravell

@mgravell

@NickCraver

@NickCraver

NickCraver

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 || NETSTANDARD2_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.

@NickCraver 

1. remove RedisKey.CompositeEquals - prefer CopyTo
2. use [SkipLocalsInit]

@mgravell 

2. fix broken RedisKey.GetHashCode() !!!

@mgravell

@mgravell

@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:

  1. a string compared (hash-code) to the utf-8 bytes of the same string
  2. a prefix/suffix pair compared (hash-code) to the combined single value

With the revisions: Equals, ==, !=, GetHashCode all behave correctly now.

@mgravell

@NickCraver

NickCraver

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!

@NickCraver

@NickCraver

@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?

@mgravell

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.