Optimize InternableString.GetHashCode

[ladipro]

Context

The current straightforward implementation is slow, especially on 64-bit Framework CLR. It is causing a measurable regression when evaluating large projects.

Changes Made

Rewrote the routine using a similar approach as this BCL method. Calculating only 2 and not 4 characters at a time, though, to reduce the complexity since our implementation works on a list of spans and not just one string. The additional perf benefit of going 4 at a time would be relatively small.

Testing

• A new unit test to verify correctness.
• Micro-benchmark showing 1.6x boost on x86 and 2x boost on x64.
• Evaluation perf traces showing significant reduction in the time spent in GetHashCode, saving 110 ms (~6%) per evaluation of the Unreal Engine C++ project on 64-bit.

[Therzok]

slightly off-topic: Wouldn't caching this string's hashcode result in a net improvement?

[ladipro]

GetHashCode runs only once for a given string unless the caller calls SpanBasedStringBuilder.ToString() multiple times on the same instance without mutating it between the calls. Same as calling StringBuilder.ToString() multiple times, it is technically possible but enough of an anti-pattern that the implementation does not cache the result.

More on running GetHashCode only once: When a string is added to the weak cache, its hash code is used as a key in a dictionary. So it's not calculated on each look-up because the look-up is done based on the hash code and not the string itself. When we're looking for a string in the weak cache, we calculate its hash code once per lookup and, as argued above, there should not be more than one look-up for the same string.

[rainersigwald]

Looks reasonable to me but since hashing is scary just wanted to check on the algo and constants and things.

I also note in the linked impl that it's "when collisions aren't a problem", which . . . seems right for us but have you thought about that in depth?

[ladipro]

Looks reasonable to me but since hashing is scary just wanted to check on the algo and constants and things.

Indeed, I have a major bug there because << is a bit shift and I need bit rotation 🤦‍♂️

The plan:

• Fix the code to use BitOperations.RotateLeft.
• Re-run the benchmark.
• Fix the unit tests which explicitly uses colliding strings.
• Check for collisions on a representative set of strings as a sanity check.

I also note in the linked impl that it's "when collisions aren't a problem", which . . . seems right for us but have you thought about that in depth?

This was discussed when the current implementation was being developed and we convinced ourselves that since you can ask MSBuild to wipe the drive or do anything the current user can do, we don't really worry about DoS'ing it with crafted strings. In other words, strings processed by MSBuild are not considered user input that we should be protecting against because the whole build process has to be trusted by design.

[ladipro]

The perf win on my VM is now lower - was 4.5x, now only 2x. I see the same thing without the new commits so it's likely because of the particular physical CPU where my machine is hosted is different. I have updated the description with the new numbers.

[rokonec]

Please avoid using ref variables in tight loops. JIT cant optimize it into registry.
I recommend to change signature to private static unsafe uint GetHashCodeHelper(char* charPtr, int length, uint hash, ref bool hashedOddNumberOfCharacters) and call it hash = GetHashCodeHelper(charPtr, span.Length, hash, ref hashedOddNumberOfCharacters);

In my micro benchmark, this simple change makes it about 2x faster.

[ladipro]

Wow, this must be the reason why it got slower after the last update. Confirming your results, it really is more than 2x faster after eliminating the ref parameter.

[ladipro]

#6845

[Forgind]

Would it also be faster using out parameters? Having an input and out parameter that happen to match. Or maybe returning a tuple?

[rokonec]

@Forgind Using non ref local variable as running hash in tight loop and than copy it to out parameter would most probably render about same benefit. However, returning integer value from procedure is something highly optimized by calling conventions. In particular it returns value in registry eax. This is significantly faster than exchanging return values by copying it into stack memory which both value tuple and out variables does. By significant I mean about 1 us slower for modern CPUs, so in practical world it rarely matters.