Add RedisValue.EqualityComparer, with an opt-in binary reading - #3230
Merged
Merged
Conversation
mgravell
force-pushed
the
marc/redisvalue-binary-comparer
branch
from
September 17, 2026 08:40
9e2ab39 to
de7db90
Compare
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
force-pushed
the
marc/redisvalue-binary-comparer
branch
from
September 17, 2026 09:01
de7db90 to
119431f
Compare
…gence 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.
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.
2 tasks
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.
Stacked on #3228; retarget to
mainif that lands first. Replaces the approach in #3229, which I closed.Why this shape
RedisValueequality works on the text a value decodes to — that is what lets a blob equal the string itspells, and
GetHashCodehashes the same decoded form to stay consistent with it. For callers moving largepayloads that reading costs more than they need.
#3229 tried making the type itself cheaper, and #3116 tried making its
Equalsbyte-based. Both run into thesame wall:
RedisValue.Equals(object)accepts astring, andRedisValue.GetHashCode()currently agrees withstring.GetHashCode()for the same text, so aHashtablekeyed byRedisValueand probed bystringworkstoday. Changing either half silently breaks that, and it is not a change that belongs outside a major.
A comparer sidesteps it entirely: it owns both halves of its own contract, so the byte reading is
self-consistent within it, and nothing changes for anyone who does not ask.
Shape
An abstract base with
DefaultandBinary, implementingIEqualityComparer<RedisValue>and the untypedIEqualityComparer. The untyped side accepts whateverRedisValueitself accepts fromobject—string,byte[], the numerics — through the same forgivingTryParsethatEquals(object)uses, rather thandemanding a boxed
RedisValue.Derivation is closed via a
private protectedconstructor. Nobody needs to derive from this to write their ownrules —
IEqualityComparer<RedisValue>is right there — and leaving it open would fix the shape permanently.(
closedwould say this more directly but is C# 15; we are on 14.)Equality only, deliberately. A total order would have to choose between the numeric, textual and raw-byte
readings of a value, and those disagree for the same logical value. Redis does not define one either: sorted
sets order by score, and
ZRANGEBYLEXorders member bytes rather than values. If ordering is ever wanted itshould be named for its domain rather than offered as a general "comparer".
Where Binary differs from Default
It agrees with
Defaultfor text that is well-formed and not numeric, and differs in three places:operator ==runsSimplify()first, so"1.0","1.00"and"1"are all equal underthe default rules, as are
"0"and"-0.0"and"1e2"/"100".Binarycompares the bytes, so they arenot — which is how the server identifies members.
Binarycalls those equal and
Defaultdoes not.0xFF, which decodes to U+FFFD) isdistinct from that text under
Binary, whereDefaultcalls them equal.Each of the three arrived the same way: a differential test failing on a case I had not thought of, after
documentation that claimed fewer of them. All three are now pinned.
A property worth having: Binary does not depend on storage
Binaryreads the UTF-8 form of both sides, so the same text equals itself however it arrived. The defaultrules do not manage that for every input — see #3233, where a string and the bytes of that same string can
simplify differently, to the point that a value written with
StringSetdoes not equal the value read back byStringGet.Binaryis unaffected, and there is now a test holding that.Numbers
Against
Defaulton the same matrix (net10.0, string vsbyte[]):DefaultBinaryHashing is the bigger story, since it never decodes: 30.4 → 4.5 ns (16B), 503.7 → 28.2 ns (1KB),
31,026 → 884 ns (64KB, 35×). Blob-against-blob: 34.6 → 5.8 ns at 16B, and 38.7 → 5.6 ns for an early
mismatch at 64KB.
Two defects the benchmarks caught that the tests could not have:
Binarywas initially 61× slower thanDefaultfor an early mismatch at 64KB (3,213 ns vs 52 ns),because it materialised both byte forms before comparing anything. It now encodes the string a chunk at a
time directly onto the blob's own bytes.
GetByteCount()precheck was the floor rather than the comparison — measuring a string's UTF-8 lengthwalks all of it. Moving it behind the fast paths took 64KB/DiffAtStart from 897 ns to 15.5 ns.
Testing
Defaultis asserted to match the type's own==andGetHashCodeexactly, so it is a true no-op wrapper.Binaryis asserted to agree withDefaulton everything that round-trips, to diverge in exactly the twodocumented ways, to be self-consistent (equal implies equal hash) across malformed UTF-8 and segmented blobs,
and to work as a
Dictionarycomparer. The untyped API is covered forstring/byte[]/RedisValueand forinputs it cannot read.
Full suite: 6372 passed, 0 failed. All six TFMs build with 0 warnings.
Review round
An external review found a real defect that my tests could not reach, plus a documentation error. Both fixed
here; recording them because the first is the kind of thing worth knowing about the shape of the code.
The chunk-boundary surrogate guard was wrong.
StringEqualsBytesencodes 512 characters at a time, andthe guard against splitting a surrogate pair extended the chunk by one character. That fails twice over: 511
three-byte characters followed by a straddling pair need 1537 bytes of a 1536-byte buffer, so
Encoding.UTF8.GetBytesthrowsArgumentException; and where the character at the seam is itself a lone highsurrogate, the extra character is the following pair's high half, so a real pair gets split and an equal
value compares unequal. Stepping back instead fixes both, and cannot reach zero because it is only reachable
when
take == ChunkChars.Fuzzing a 500–530 character corpus over the awkward alphabet: 247 wrong in 20,000 before, 0 after, no
exceptions either side of the fix.
The tests could not have caught it — the longest value in the corpus was 300 characters, so the chunking loop
never ran past its first pass. There are now cases placing 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.
Also from that review: numeric divergence was missing from the remarks entirely (above); the seed now comes
from
RandomNumberGeneratorrather thanGuid.NewGuid, whose version and variant bits are fixed; and twoordinally-equal strings now take a cheap exit, since identical text encodes identically.
Declined: a shortcut comparing same-type numbers on their raw values, because negative zero formats
differently from how it compares, and byte semantics are the entire point of
Binary.