Conversation
Share bounded byte-based hashing between string and span entrypoints, with ASCII casing specialization and ordinal Unicode fallback. Preserve randomized Marvin hashing and handle oversized UTF-16 inputs safely. Remove tests coupled to private comparer implementations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15b6da83-d0d6-4c6c-9705-88058bc8cc17
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
Remove redundant unchecked contexts and use generic casing masks directly instead of explicit type guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15b6da83-d0d6-4c6c-9705-88058bc8cc17
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Multiple moderate issues require restored coverage, SIMD/scalar validation, and a less costly non-ASCII fallback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces duplicated non-randomized string hashing with a shared, memory-safe SIMD implementation.
Changes:
- Adds unified ordinal and ordinal-ignore-case hashing.
- Removes the previous unsafe implementations.
- Removes several comparer, hash-equivalence, and collision-transition tests.
File summaries
| File | Review |
|---|---|
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs |
Removes string/span hash-equivalence coverage. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringComparerTests.cs |
Restore reflected internal comparer cases and helper to preserve unwrapping-branch coverage. Moderate; 2 votes. |
src/libraries/System.Private.CoreLib/src/System/String.NonRandomizedHashCode.cs |
Add SIMD/scalar parity and bounded-memory tests; avoid the redundant non-ASCII source buffer; provide representative benchmarks. Two moderate and one nit; 2 votes each. |
src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs |
Removes the previous hash implementations. |
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems |
Includes the new hash implementation source. |
src/libraries/System.Collections/tests/Generic/Dictionary/HashCollisionScenarios/OutOfBoundsRegression.cs |
Replace, rather than remove, collision-threshold and comparer-upgrade coverage. Moderate; 2 votes. |
src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionary.Generic.Tests.cs |
Retain equivalent coverage for randomized-comparer upgrades during collisions. Moderate; 2 votes. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Balanced
Comment on lines
41
to
43
| protected override string CreateTValue(int seed) => CreateTKey(seed); | ||
|
|
||
| [Theory] | ||
| [InlineData(false)] | ||
| [InlineData(true)] | ||
| public void NonRandomizedToRandomizedUpgrade_FunctionsCorrectly(bool ignoreCase) | ||
| { | ||
| List<string> strings = GenerateCollidingStrings(110); // higher than the collisions threshold | ||
|
|
||
| var cd = new ConcurrentDictionary<string, string>(ignoreCase ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); | ||
| for (int i = 0; i < strings.Count; i++) | ||
| { | ||
| string s = strings[i]; | ||
|
|
||
| Assert.True(cd.TryAdd(s, s)); | ||
| Assert.False(cd.TryAdd(s, s)); | ||
|
|
||
| for (int j = 0; j < strings.Count; j++) | ||
| { | ||
| Assert.Equal(j <= i, cd.ContainsKey(strings[j])); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static List<string> GenerateCollidingStrings(int count) | ||
| { | ||
| static Func<string, int> GetHashCodeFunc(ConcurrentDictionary<string, string> cd) | ||
| { | ||
| // If the layout of ConcurrentDictionary changes, this will need to change as well. | ||
|
|
||
| FieldInfo tablesField = AssertNotNull(typeof(ConcurrentDictionary<string, string>).GetField("_tables", BindingFlags.Instance | BindingFlags.NonPublic)); | ||
| Type tablesType = Type.GetType("System.Collections.Concurrent.ConcurrentDictionary`2+Tables, System.Collections.Concurrent", throwOnError: true); | ||
| object tables = AssertNotNull(tablesField.GetValue(cd)); | ||
|
|
||
| FieldInfo comparerField = AssertNotNull(tablesType.GetField("_comparer", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public)); | ||
| comparerField = AssertNotNull((FieldInfo)tables.GetType().GetMemberWithSameMetadataDefinitionAs(comparerField)); | ||
| IEqualityComparer<string> comparer = AssertNotNull((IEqualityComparer<string>)comparerField.GetValue(tables)); | ||
|
|
||
| return comparer.GetHashCode; | ||
|
|
||
| static T AssertNotNull<T>(T value, [CallerArgumentExpression(nameof(value))] string valueArg = null) | ||
| { | ||
| Assert.True(value is not null, valueArg); | ||
| return value; | ||
| } | ||
| } | ||
|
|
||
| Func<string, int> nonRandomizedOrdinal = GetHashCodeFunc(new ConcurrentDictionary<string, string>(StringComparer.Ordinal)); | ||
| Func<string, int> nonRandomizedOrdinalIgnoreCase = GetHashCodeFunc(new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase)); | ||
|
|
||
| const int StartOfRange = 0xE020; // use the Unicode Private Use range to avoid accidentally creating strings that really do compare as equal OrdinalIgnoreCase | ||
| const int Stride = 0x40; // to ensure we don't accidentally reset the 0x20 bit of the seed, which is used to negate OrdinalIgnoreCase effects | ||
| int currentSeed = StartOfRange; | ||
|
|
||
| List<string> collidingStrings = new List<string>(count); | ||
| while (collidingStrings.Count < count) | ||
| { | ||
| Assert.True(currentSeed <= ushort.MaxValue, | ||
| $"Couldn't create enough colliding strings? Created {collidingStrings.Count}, needed {count}."); | ||
|
|
||
| // Generates a possible string with a well-known non-randomized hash code: | ||
| // - string.GetNonRandomizedHashCode returns 0. | ||
| // - string.GetNonRandomizedHashCodeOrdinalIgnoreCase returns 0x24716ca0. | ||
| // Provide a different seed to produce a different string. | ||
| // Must check OrdinalIgnoreCase hash code to ensure correctness. | ||
| string candidate = string.Create(8, currentSeed, static (span, seed) => | ||
| { | ||
| Span<byte> asBytes = MemoryMarshal.AsBytes(span); | ||
|
|
||
| uint hash1 = (5381 << 16) + 5381; | ||
| uint hash2 = BitOperations.RotateLeft(hash1, 5) + hash1; | ||
|
|
||
| MemoryMarshal.Write(asBytes, in seed); | ||
| MemoryMarshal.Write(asBytes.Slice(4), in hash2); // set hash2 := 0 (for Ordinal) | ||
|
|
||
| hash1 = (BitOperations.RotateLeft(hash1, 5) + hash1) ^ (uint)seed; | ||
| hash1 = (BitOperations.RotateLeft(hash1, 5) + hash1); | ||
|
|
||
| MemoryMarshal.Write(asBytes.Slice(8), in hash1); // set hash1 := 0 (for Ordinal) | ||
| }); | ||
|
|
||
| int ordinalHashCode = nonRandomizedOrdinal(candidate); | ||
| Assert.Equal(0, ordinalHashCode); // ensure has a zero hash code Ordinal | ||
|
|
||
| int ordinalIgnoreCaseHashCode = nonRandomizedOrdinalIgnoreCase(candidate); | ||
| if (ordinalIgnoreCaseHashCode == 0x24716ca0) // ensure has a zero hash code OrdinalIgnoreCase (might not have one) | ||
| { | ||
| collidingStrings.Add(candidate); // success! | ||
| } | ||
|
|
||
| currentSeed += Stride; | ||
| } | ||
|
|
||
| return collidingStrings; | ||
| } | ||
| } |
| { | ||
| #region Dictionary | ||
| public class InternalHashCodeTests_Dictionary_NullComparer : InternalHashCodeTests<Dictionary<string, string>> | ||
| public class InternalHashCodeTests_Dictionary_NullComparer |
Comment on lines
+187
to
+191
| if (Vector128.IsHardwareAccelerated) | ||
| { | ||
| Vector128<uint> hash = initialLength == byteLength | ||
| ? Vector128.Create(length) + Vector128.Create(HashPrime1, HashPrime2, HashPrime3, HashPrime4) | ||
| : Vector128.Create(h0, h1, h2, h3); |
Comment on lines
+262
to
+264
| char[]? borrowedSource = null, borrowedScratch = null; | ||
| Span<char> source = (uint)length < 256 ? stackalloc char[256] : | ||
| (borrowedSource = ArrayPool<char>.Shared.Rent(length)); |
Comment on lines
175
to
178
| RunTest(StringComparer.OrdinalIgnoreCase, true, true); | ||
| RunTest(StringComparer.InvariantCulture, false, false); // not ordinal | ||
| RunTest(StringComparer.InvariantCultureIgnoreCase, false, false); // not ordinal | ||
| RunTest(GetNonRandomizedComparer("WrappedAroundDefaultComparer"), true, false); // EC<string>.Default is Ordinal-equivalent | ||
| RunTest(GetNonRandomizedComparer("WrappedAroundStringComparerOrdinal"), true, false); | ||
| RunTest(GetNonRandomizedComparer("WrappedAroundStringComparerOrdinalIgnoreCase"), true, true); | ||
| RunTest(new CustomStringComparer(), false, false); // not an inbox comparer |
Use two independent Vector128 ulong accumulators and a shift/add/xor recurrence over 32-byte blocks. Keep the scalar path and Unicode fallback continuation consistent with the new state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15b6da83-d0d6-4c6c-9705-88058bc8cc17
This was referenced Sep 8, 2026
Inputs of 9..32 bytes previously left the inlined fast path and called GetNonRandomizedHashCodeLarge, which is where the 5..16 char regression against main came from. Handle that range in place and only call out above 32 bytes. Also fold MixNonRandomizedHash down to a single multiply. Since x * K + y * K == (x + y) * K, mixing b in at two rotations gives the same spread as the previous two multiplies while needing one 64-bit constant instead of two - each one costs a movz plus three movk on Arm64. Bucket distribution measured equal or better across random, counter-suffixed, path-like and dotted-identifier key sets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
Member
Author
|
@EgorBot -linux_amd -osx_arm64 -linux_arm64 using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class Benchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() =>
Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
_key = new string('a', Length);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
} |
Combining the length linearly cancels against a character whose delta matches the length delta. In the 5..8 byte path first ^ (uint)length collides every pair like "600"/"8000", since '6'^6 == '8'^8 and the trailing word is identical; the 9..32 byte path had the same problem through a += (uint)length. Over 200k decimal keys that is 507 collisions where main has none. Multiplying the length by a prime first leaves 3, against ~4.7 expected for a random 32-bit hash. Across 24 key sets total collisions drop from 715 to 236 (~120 is the random baseline). The multiply does not depend on any load, so it costs one instruction on the 4-char path and nothing elsewhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
The block loop is bound by its dependent chain, not by throughput, so the way to go faster is to absorb more bytes per step rather than to issue fewer instructions. Widening the state from four 64-bit lanes to eight doubles the bytes per step and folds back to four afterwards, so the tail and the ignore-case continuation are unchanged. Lane i always takes the ulong at byte offset 8 * i. That layout falls out of the natural loads at every width, so one Vector512, two Vector256, four Vector128 and the scalar loop all produce the same hash; verified by checksumming ~7300 strings under PreferredVectorBitWidth 512, 256 and 128 and with EnableHWIntrinsic=0. Hashing 16000 chars goes from 3763 to 1948 cycles on Zen 4, 8.1 to 16.4 bytes per cycle, and the dictionary lookup it was measured through from 7031 to 5072. Seeding and folding eight lanes needs enough blocks to pay for itself, hence the threshold; below it the existing four-lane loop still runs. Collision behaviour is unchanged: 236 to 230 total over 24 key sets, identical worst-case probe lengths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
Member
Author
|
@EgorBot -linux_amd -osx_arm64 -linux_arm64 using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class Benchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() =>
Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
_key = new string('a', Length);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
} |
This reverts commit 09f5b62.
The ignore-case slow path copied the bytes into a char buffer, upper cased them into a second buffer, walked that buffer once more to or in 0x20, and only then hashed it. Four passes and two pooled buffers, where main manages two passes and one, and none of the benchmarks noticed because they all hash strings of 'a'. On this machine hashing Cyrillic, CJK or Latin-1 text cost 1.14x to 1.61x of main. The bytes always originate from a char span, so reinterpreting them removes the copy and one of the buffers. Hashing the upper cased text under a casing that ors in 0x20 but rejects nothing removes the third pass, which is how main folds it in too. Two passes, one buffer, and 0.91x to 0.96x of main on the same inputs. Hash values are unchanged. The remaining span handed to ToUpperOrdinal stays within int.MaxValue / sizeof(char) because the entry points route anything longer through the chunked path, and the destination is sliced to the length actually needed. Checked either side of that boundary with the non-ASCII character first, in the middle and last: no OverflowException in any of the twelve combinations, where main throws in six of them because it hands ToUpperOrdinal an unsliced rented buffer whose length can exceed what MemoryMarshal.AsBytes accepts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
Member
Author
|
@EgorBot -linux_amd -osx_arm64 -linux_arm64 using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(ASCIIBenchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class ASCIIBenchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() => Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
_key = new string('a', Length);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
}
[MemoryDiagnoser]
public class NonASCIIBenchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() => Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
// One Cyrillic letter in the middle, the rest ASCII.
char[] chars = new char[Length];
Array.Fill(chars, 'a');
chars[Length / 2] = '\u0416';
_key = new string(chars);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
} |
NonRandomizedGetHashCode_EquivalentForStringAndSpan checks that the string and span entry points agree, which is one of the invariants this PR is meant to preserve, and it compares the two entry points against each other rather than against fixed values, so it is unaffected by the hash changing. Deleting it removed coverage of exactly what changed. The StringComparerTests cases are unrelated to hashing: they feed the NonRandomizedStringEqualityComparer instances to IsWellKnownOrdinalComparer and IsWellKnownCultureAwareComparer, and are the only inputs that reach the IInternalStringEqualityComparer unwrapping branch in either. Both pass unmodified against the new implementation. The remaining deleted suites are the ones built on strings chosen to collide under the old hash, which need a new generator rather than a straight restore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
The block loop is bound by its dependent chain, not by throughput, so the way to go faster is to absorb more bytes per step rather than to issue fewer instructions. Widening the state from four 64-bit lanes to eight doubles the bytes per step and folds back to four afterwards, so the tail and the ignore-case continuation are unchanged. Lane i always takes the ulong at byte offset 8 * i. That layout falls out of the natural loads at every width, so one Vector512, two Vector256, four Vector128 and the scalar loop all produce the same hash; verified by checksumming ~7300 strings under PreferredVectorBitWidth 512, 256 and 128 and with EnableHWIntrinsic=0. It lives in its own method rather than inline. Inlining it into the four-lane loop, which is itself inlined into its caller, pushed that caller from 441 to 771 bytes and gave it a stack frame, and every input between 65 and 255 bytes paid that prologue without ever reaching the wide loop: 18 extra instructions per lookup. Out of line the mid-range is within two instructions of not having this at all. Passing the loop state by reference instead has the opposite problem, keeping the accumulators in memory across every iteration. Seeding and folding eight lanes needs enough blocks to pay for itself, hence the threshold; below it the four-lane loop still runs. Hashing 16000 chars goes from 3616 to 1830 cycles on Zen 4, and 1000 chars from 364 to 229. Collision behaviour is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
The collision suites were deleted because their keys were constructed by inverting the old hash. Replace the generator rather than the tests: for an eight char string the hash is a function of two 64-bit halves, so pinning the value they combine into pins the hash, which lets the second half be chosen freely and the first solved for. Keys are drawn from the Private Use Area with bit 0x20 set, which is uncased and already carries the bit ignore-case hashing ORs in, so each key hashes the same under both comparers and no two compare equal OrdinalIgnoreCase. The generator checks its output against the real hash, so if the algorithm changes again the failure points at the generator rather than at every test. That restores the comparer upgrade, rebucketing, alternate lookup, public comparer and serialization coverage for Dictionary, HashSet, OrderedDictionary and ConcurrentDictionary. Also add the missing coverage for the invariant this change is built on: the hash must not depend on the vector width. The test checksums lengths straddling every size branch under both casings, then repeats it in a child process with hardware intrinsics disabled and requires the same answer. The child asserts intrinsics really are off first, so it cannot pass by silently running the vectorized path twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3657f258-533a-4a30-9586-e0cd40449577
Member
Author
|
@EgorBot -linux_amd -osx_arm64 -linux_arm64 using System;
using System.Collections.Generic;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(ASCIIBenchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class ASCIIBenchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() => Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
_key = new string('a', Length);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
}
[MemoryDiagnoser]
public class NonASCIIBenchmarks {
private Dictionary<string, int> _dictionary = null!;
private string _key = null!;
[Params(1, 4, 10, 16, 20, 50, 100, 1000, 16000)]
public int Length { get; set; }
[GlobalSetup(Target = nameof(Ordinal))]
public void SetupOrdinal() => Setup(StringComparer.Ordinal);
[GlobalSetup(Target = nameof(OrdinalIgnoreCase))]
public void SetupOrdinalIgnoreCase() => Setup(StringComparer.OrdinalIgnoreCase);
private void Setup(StringComparer comparer) {
// One Cyrillic letter in the middle, the rest ASCII.
char[] chars = new char[Length];
Array.Fill(chars, 'a');
chars[Length / 2] = '\u0416';
_key = new string(chars);
_dictionary = new Dictionary<string, int>(1024, comparer) {
[new string(_key.AsSpan())] = 42
};
}
[Benchmark]
public int Ordinal() => _dictionary[_key];
[Benchmark]
public int OrdinalIgnoreCase() => _dictionary[_key];
} |
This was referenced Sep 9, 2026
Open
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR changes the hash implementation for the following internal APIs:
via one SIMD-friendly implementation, fully memory safe.
The previous one duplicated (not even sure
GetNonRandomizedHashCode()andGetNonRandomizedHashCode(ReadOnlySpan<char> span)were the same). Relied on unsafe code + did intentional out of bounds read with the assumption about string's zero termnator char. And most importantly, were not SIMD friendly.The hash code impl should not change its output depending on whether SIMD is enabled or disabled.
Throughput
Dictionary<string, int>lookup by an equal-but-not-reference-equal key (benchmark, full results). Numbers are speedup vsmain, higher is better. The non-ASCII columns use the same key with a single Cyrillic letter in the middle, which is what forces the ignore-case slow path.StringComparer.OrdinalASCII
non-ASCII
ASCII
non-ASCII
ASCII
non-ASCII
StringComparer.OrdinalIgnoreCaseASCII
non-ASCII
ASCII
non-ASCII
ASCII
non-ASCII
Note
Three microarchitectures are shown because the block loop is latency-bound: its recurrence costs
chain_depth × vector-ALU-latency, and that latency is 1 cycle on Zen 4 but 2 cycles on Zen 5, Apple silicon and Neoverse N2. An earlier revision that ignored this was up to 1.38x slower thanmainon long strings despite being faster on Zen 4. The loop now runs two independent accumulators over 32-byte blocks with a 3-deep recurrence, so it wins on both.The worst case anywhere is 0.89x, under two nanoseconds, in the 4-20 character range where
main's plain 8-byte loop is hard to beat. Only compare within a column: these runs come from shared machines and the absolute timings move between runs.Collision analysis
Verified against the real implementations (bound by reflection, so the analyzer runs unmodified on both builds) rather than a model, over 24 key sets x 200k keys: random at many lengths, counters, prefix+counter, suffix+counter, path-like keys, dotted type names, GUIDs, hex, single-character differences, sparse bit differences, repeated characters, Cyrillic and CJK, and case variants. A reference-quality hash (FNV-1a + murmur3 finalizer) is included as a control, so a weak hash can be told apart from a bad measurement.
mainProbe length is the number of chain entries visited for a successful lookup. Against the prime modulus that
DictionaryandHashSetactually use,mainand this PR are equivalent, so real dictionary behaviour does not regress. The difference is in the low bits, which matters for anything that masks instead of taking a remainder, and there this PR is a large improvement.maincollides whole families of realistic keys: single-character differences at a fixed position degrade its power-of-two probe length to 913, andprefix + counterkeys to 125.Neither hash is avalanche-quality, by design; both are speed-first, and the prime modulus is what keeps that safe.
One collision family found while measuring was specific to this PR and has been fixed: combining the length into the hash linearly cancels against a character whose delta matches the length delta, so
first ^ (uint)lengthcollided every pair like"600"/"8000"('6' ^ 6 == '8' ^ 8) with an identical trailing word. That was 507 collisions over 200k decimal keys wheremainhas none. Multiplying the length by a prime before mixing leaves 3, against ~4.7 expected for a random 32-bit hash, and costs one instruction on the 4-character path and nothing elsewhere.Behaviour on very long inputs
MemoryMarshal.AsBytesthrowsOverflowExceptiononce a char span exceedsint.MaxValue / 2elements, so working in bytes needs care. The entry points route anything longer through a chunked path, and the ignore-case slow path slices its scratch buffer to the length actually needed. Checked either side of that boundary with the non-ASCII character first, in the middle and last, for both comparers: no exception in any of the twelve combinations.mainthrows in six of them, fromAscii.ChangeCasehandingMemoryMarshal.AsBytesan unslicedArrayPoolbuffer whose length can exceed the limit. That is a pre-existing issue independent of this PR.