Skip to content
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

Merged
merged 15 commits into from
Apr 21, 2022
Merged

Conversation

mgravell
Copy link
Collaborator

@mgravell mgravell commented Apr 19, 2022

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?

Copy link
Collaborator

@NickCraver NickCraver left a 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.

case string s:
if (s.Length != 0)
{
#if NETCOREAPP3_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

@mgravell
Copy link
Collaborator Author

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

Copy link
Collaborator

@NickCraver NickCraver left a 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!

@NickCraver
Copy link
Collaborator

@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
Copy link
Collaborator Author

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.

@mgravell mgravell merged commit e74076d into main Apr 21, 2022
@mgravell mgravell deleted the slot-efficiency branch April 21, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants