Conversation
Hashing a RedisValue ran at roughly 2 GB/s, so any large value spent almost all of its hash time inside the hash function itself - the UTF8 decode that sits alongside it for byte-backed values accounted for under 4% of the total. At 64KB that is ~31us to produce a single int. Use XxHash3 over the decoded characters instead, seeded once per process. System.IO.Hashing is already a dependency and XxHash3 is already used by ValueCondition, so this costs nothing in packaging, and it lets the down-level targets take the same path as the rest rather than falling back to building a string. 64KB blob 31,026ns -> 2,970ns (10.4x) 64KB string 29,885ns -> 1,762ns (17.0x) 1KB blob 504ns -> 85ns (5.9x) 1KB string 490ns -> 69ns (7.1x) 16B ~30ns -> ~28ns The whole content is still hashed - no sampling - so equal values continue to land in the same bucket by construction, whatever representation they arrived in. What is given up is that Marvin is a *keyed* hash, designed so collisions cannot be constructed without the key, while xxHash's seed carries no such guarantee: it provides per-process unpredictability, not resistance. That is a deliberate trade on the basis that a RedisValue is rarely a dictionary key and practically never an attacker-chosen one. Note that RedisValue.GetHashCode() no longer agrees with String.GetHashCode() for the same text; it did before, because the string case forwarded straight to it. Hash codes have always been per-process and must never be persisted, so nothing may depend on the value, but the coincidence is gone.
Collaborator
Author
|
Closing — this is a breaking change to shipped behaviour, and it does not belong in a minor. The specific trap, verified rather than theorised. So after this change The measured win was real (10-17x, and more for long strings than blobs), so the finding stands even though Superseded by the opt-in approach: a #3228 is unaffected and still stands on its own: it changes only how |
This was referenced Sep 16, 2026
mgravell
added a commit
that referenced
this pull request
Sep 17, 2026
Comparing and hashing a RedisValue works on the text a value decodes to, which is what lets a blob equal the string it spells. For callers slinging large payloads that reading costs more than they want, and for them the bytes would do just as well. Rather than change what the type itself means - see #3229, where doing so turned out to break a Hashtable keyed by RedisValue and probed by string, silently - offer the byte reading as something a caller asks for: new Dictionary<RedisValue, T>(RedisValue.EqualityComparer.Binary) An abstract base with Default and Binary, implementing both the typed and the untyped comparer interfaces; the untyped side accepts anything RedisValue itself accepts from object - string, byte[], the numerics - via the same forgiving TryParse that Equals(object) uses. Derivation is closed via a private protected constructor: nobody needs to derive from this to write their own rules, and leaving it open would fix the shape forever. Deliberately equality only. A total order would have to choose between the numeric, textual and raw-byte readings of a value, and those disagree; Redis does not define one either, since sorted sets order by score and ZRANGEBYLEX orders member bytes. Binary agrees with Default wherever a value's UTF8 form round-trips - all well-formed text - and differs where it does not, in both directions: a string holding an unpaired surrogate encodes as U+FFFD does, and a blob that is not canonical UTF8 decodes to U+FFFD without re-encoding to itself. Both its equality and its hashing read the same bytes, so it stays self-consistent; that is what makes the byte reading safe here and not on the type itself, whose GetHashCode hashes decoded text. Measured against Default on the same matrix (net10.0, string vs byte[]): 16B equal 56.3ns -> 10.8ns 1KB equal 131.9ns -> 41.1ns 1KB differs first 54.9ns -> 15.2ns 64KB equal 5,397ns -> 2,155ns 64KB differs first 52.4ns -> 15.5ns hash 64KB blob 31,026ns -> 884ns Two things the benchmarks caught that the tests could not: comparing a string against a blob has to encode a chunk at a time onto the blob's own bytes, or a mismatch in the first byte still costs a full pass over both sides; and the length precheck has to come after that, because measuring a string's UTF8 length walks all of it and dominates everything else.
mgravell
added a commit
that referenced
this pull request
Sep 17, 2026
Comparing and hashing a RedisValue works on the text a value decodes to, which is what lets a blob equal the string it spells. For callers slinging large payloads that reading costs more than they want, and for them the bytes would do just as well. Rather than change what the type itself means - see #3229, where doing so turned out to break a Hashtable keyed by RedisValue and probed by string, silently - offer the byte reading as something a caller asks for: new Dictionary<RedisValue, T>(RedisValue.EqualityComparer.Binary) An abstract base with Default and Binary, implementing both the typed and the untyped comparer interfaces; the untyped side accepts anything RedisValue itself accepts from object - string, byte[], the numerics - via the same forgiving TryParse that Equals(object) uses. Derivation is closed via a private protected constructor: nobody needs to derive from this to write their own rules, and leaving it open would fix the shape forever. Deliberately equality only. A total order would have to choose between the numeric, textual and raw-byte readings of a value, and those disagree; Redis does not define one either, since sorted sets order by score and ZRANGEBYLEX orders member bytes. Binary agrees with Default wherever a value's UTF8 form round-trips - all well-formed text - and differs where it does not, in both directions: a string holding an unpaired surrogate encodes as U+FFFD does, and a blob that is not canonical UTF8 decodes to U+FFFD without re-encoding to itself. Both its equality and its hashing read the same bytes, so it stays self-consistent; that is what makes the byte reading safe here and not on the type itself, whose GetHashCode hashes decoded text. Measured against Default on the same matrix (net10.0, string vs byte[]): 16B equal 56.3ns -> 10.8ns 1KB equal 131.9ns -> 41.1ns 1KB differs first 54.9ns -> 15.2ns 64KB equal 5,397ns -> 2,155ns 64KB differs first 52.4ns -> 15.5ns hash 64KB blob 31,026ns -> 884ns Two things the benchmarks caught that the tests could not: comparing a string against a blob has to encode a chunk at a time onto the blob's own bytes, or a mismatch in the first byte still costs a full pass over both sides; and the length precheck has to come after that, because measuring a string's UTF8 length walks all of it and dominates everything else.
mgravell
added a commit
that referenced
this pull request
Sep 17, 2026
* Add RedisValue.EqualityComparer, with an opt-in binary reading Comparing and hashing a RedisValue works on the text a value decodes to, which is what lets a blob equal the string it spells. For callers slinging large payloads that reading costs more than they want, and for them the bytes would do just as well. Rather than change what the type itself means - see #3229, where doing so turned out to break a Hashtable keyed by RedisValue and probed by string, silently - offer the byte reading as something a caller asks for: new Dictionary<RedisValue, T>(RedisValue.EqualityComparer.Binary) An abstract base with Default and Binary, implementing both the typed and the untyped comparer interfaces; the untyped side accepts anything RedisValue itself accepts from object - string, byte[], the numerics - via the same forgiving TryParse that Equals(object) uses. Derivation is closed via a private protected constructor: nobody needs to derive from this to write their own rules, and leaving it open would fix the shape forever. Deliberately equality only. A total order would have to choose between the numeric, textual and raw-byte readings of a value, and those disagree; Redis does not define one either, since sorted sets order by score and ZRANGEBYLEX orders member bytes. Binary agrees with Default wherever a value's UTF8 form round-trips - all well-formed text - and differs where it does not, in both directions: a string holding an unpaired surrogate encodes as U+FFFD does, and a blob that is not canonical UTF8 decodes to U+FFFD without re-encoding to itself. Both its equality and its hashing read the same bytes, so it stays self-consistent; that is what makes the byte reading safe here and not on the type itself, whose GetHashCode hashes decoded text. Measured against Default on the same matrix (net10.0, string vs byte[]): 16B equal 56.3ns -> 10.8ns 1KB equal 131.9ns -> 41.1ns 1KB differs first 54.9ns -> 15.2ns 64KB equal 5,397ns -> 2,155ns 64KB differs first 52.4ns -> 15.5ns hash 64KB blob 31,026ns -> 884ns Two things the benchmarks caught that the tests could not: comparing a string against a blob has to encode a chunk at a time onto the blob's own bytes, or a mismatch in the first byte still costs a full pass over both sides; and the length precheck has to come after that, because measuring a string's UTF8 length walks all of it and dominates everything else. * Review: fix the chunk-boundary surrogate guard, and pin numeric divergence The guard meant to stop a surrogate pair being split across chunks extended the chunk by a character. That was wrong twice over: - it could overrun the encode buffer, which is sized for ChunkChars * 3 bytes; 511 three-byte characters followed by a straddling pair need 1537 of 1536, and Encoding.UTF8.GetBytes throws ArgumentException rather than returning a wrong answer; - where the character at the seam was itself a lone high surrogate, the extra character was the *following* pair's high half, so the split simply moved onto a real pair - each half encoding to U+FFFD, and an equal value comparing unequal. Step back instead. A chunk then never ends on a high surrogate and never exceeds ChunkChars characters, so it cannot overrun; and it is only reachable when take == ChunkChars, so it cannot reach zero. Fuzzing a 500-530 character corpus over the awkward alphabet gave 247 wrong answers in 20,000 before and 0 after, with no exceptions. The tests could not have caught this: the longest value in the corpus was 300 characters, so the chunking loop never ran past its first pass. Added cases that put a straddling pair, a lone high surrogate before a real pair, and a pair approached from the other side at exactly the 512-character seam, plus a plain 2000-character value for the multi-chunk path. Also documented and pinned a divergence that was missing: operator == runs Simplify() first, so "1.0", "1.00" and "1" are all equal under the default rules, as are "0" and "-0.0", while Binary compares the bytes and says otherwise. That is the right answer for a byte reading - it is how the server identifies members - but it belonged in the remarks, which claimed agreement for "all well-formed text". Two smaller things from the same review: seed the hash from RandomNumberGenerator rather than Guid.NewGuid, whose version and variant bits are fixed; and take the cheap exit when two strings are ordinally equal, since identical text encodes identically. The unequal case still goes the byte route, because two different strings can share a UTF-8 form. * Drop the superseded surrogate-guard comment left above its replacement * Pin that Binary does not depend on how a value is stored The default rules can simplify a string and the bytes of that same string differently (#3233), so the same text is not always equal to itself. Binary reads the UTF8 form of both sides and so is not exposed to that; asserting it keeps the property rather than leaving it as a happy accident.
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
1 participant
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.
Stacked on #3228 — take it or leave it independently; the equality work does not depend on this.
Base is
marc/redisvalue-equality-alloc, so the diff shows only the hashing change. Retarget tomainif#3228 lands first.
Why
Benchmarking hashing while working on #3228 turned up something larger than the allocation that PR removes.
HashBlobandHashStringsit within ~4% of each other at every size, which means the UTF8 decode is notthe cost — the hash function is, at roughly 2 GB/s. At 64KB that is ~31µs to produce a single
int.The change
XxHash3over the decoded characters, seeded once per process. No new dependency:System.IO.Hashingisalready referenced by the main library and
XxHash3is already used byValueCondition. It also removes the#if NETsplit, since the down-level targets no longer need to fall back to building a string.HashBlobHashStringLong strings benefit more than blobs, so this is not a blob-specific fix.
What is given up — the part that needs a decision
The whole content is still hashed. Nothing is sampled, so equal values land in the same bucket by
construction, whatever representation they arrived in, and the hash contract is safe.
What changes is the kind of hash. Marvin is keyed: collisions cannot be constructed without the key.
xxHash's seed is not a key — it gives per-process unpredictability, not resistance, and published
collision-finding techniques for xxHash do not depend on the seed. So this trades hash-flooding resistance for
speed, on the basis that a
RedisValueis rarely a dictionary key and practically never an attacker-chosenone.
That is worth weighing against the June discussion on #3116, where the leaning was the other way — towards
bringing Marvin to
RedisKey, which today uses the hand-rolled728271210-seeded hash and has no Marvin atall. This PR moves
RedisValuetowardRedisKeyrather than the reverse. Defensible, but a deliberate choicerather than a drift.
Alternatives considered and rejected:
Redis data routinely shares long prefixes, so common-prefix collisions become free. Strictly worse on the
security axis than this.
implementation.
divergence that Compare strings against blobs without decoding them into a string #3228 exists to avoid.
Note for reviewers
((RedisValue)"abc").GetHashCode()previously equalled"abc".GetHashCode(), because the string caseforwarded straight to it. It no longer does. Hash codes have always been per-process and must never be
persisted, so nothing may legitimately depend on the value — but the coincidence is gone, and a test somewhere
could be asserting it.
The decode is now the visible remainder: it was under 4% of the old cost, and at 64KB it is ~40% of the new
one (2,970 ns blob against 1,762 ns string). I would stop here rather than chase it.
Testing
Full suite: 6365 passed, 0 failed — including
EqualStringAndBlobShareHashCodesfrom #3228, which is theinvariant that matters. All six TFMs build with 0 warnings.