Optimize RedisValue / Equals

[pairbit]

I suggest converting the string to utf8 bytes and comparing the blobs. The tests work.
My position is that if a person uses strings, let their productivity suffer, not the one who chose the blobs.

I understand that the hash of a string and the hash of a string inside a RedisValue are different, but I don't think this is necessary.

var str = "my string";
RedisValue orig = str;
var hashCode = orig.GetHashCode();
Assert.That(hashCode, Is.Not.EqualTo(str.GetHashCode()));

I also found a bug in the StartsWith(ReadOnlySpan<byte> value) method for non-ASCII characters.
The test below is failing.

var str = "моя строка";
RedisValue orig = str;

Assert.That(orig.StartsWith(str), Is.EqualTo(orig.StartsWith(Encoding.UTF8.GetBytes(str))));

[mgravell]

One word: Marvin

You may already know, but "Marvin" is the randomising voodoo inside most recent string hashing / equality implementations that prevents dictionary attacks using precalculated predictable known values to force collisions (hash-flooding). Marvin introduces entropy per-process, preventing that. It is hugely desirable to retain Marvin semantics by default.

---

Longer version: if you're using BLOBs of any non-trivial value, you're unlikely to be using them as keys on dictionaries, or otherwise performing equality checks, so honestly: IMO: don't worry about it, it doesn't adversely affect you. You're probably just slinging BLOBs around, (de)serializing them, etc.

Perhaps what we really want here is a couple of pre-rolled IEqualityComparer<RedisValue> implementations: one optimized for string-like (the default), one optimized for BLOB-like (which you'd use, in your hypothetical use-case). I think giving people that option is a much safer and more versatile proposition.

Note: it would actually not be a bad idea to try to retain some semblance of Marvin in a BLOB-based implementation, but that's a stretch goal.

[pairbit]

I suggest separating the tasks. Hash calculation depends on the algorithm, and UTF-8 hash differs from Unicode hash. However, this doesn't affect the Equals and StartsWith methods.

[pairbit]

Another question is, why don't you use Marvin in RedisKey?

[mgravell]

Fair question. I suspect there's a lot of validity in the suggestion here. We need to be careful, but: string -> byte is at least well defined, where-as byte -> string isn't (in the general case).

I might see if I can drag the private Marvin core over - I did the same when over at aspnet for netfx, so I'll see if I can find it.

[pairbit]

The method int AddHashCode(ReadOnlySpan<byte> span, int acc) did not work correctly with Sequence, so I rolled back the changes and left only Equals

[pairbit]

The GetHashCode question turned out to be complicated, so I suggest not considering it in this PR

[mgravell]

Happy to keep looking at this, but I think it needs some planning and thought, and when we're talking about "optimize", proving that might we worthwhile; I've (separately) done some benchmark comparing System.Text.Encoding.UTF8 with System.Text.Unicode.Utf8 - summary: they use the same core, and it isn't worth getting excited about the delta (which would also need an OOB package for down-level), so: don't touch that. The core idea is sound.

[pairbit]

Ok. I understand. It's not a matter of principle. Am I closing the merge?

[pairbit]

What about the bug in the method bool StartsWith(ReadOnlySpan<byte> value)?

// BUG if Not ASCII
if (s.Length < value.Length) return false; // not enough characters to match

Test failed:

var str = "моя строка";
RedisValue orig = str;

Assert.That(orig.StartsWith(str), Is.EqualTo(orig.StartsWith(Encoding.UTF8.GetBytes(str))));

[mgravell]

Don't close it - and yes, the bug needs fixing; I'm just setting expectations that this is going to need some full attention.

[mgravell]

Closing this at last, and I owe you both an apology for the delay and a proper explanation, because you were right about the problem — more right than I gave you credit for in June.

When I said "if you're slinging BLOBs around you're unlikely to be performing equality checks, so honestly, don't worry about it" — I was wrong, and I only know that because your PR made me go and measure. Mixed string/blob equality allocates a transient string proportional to the payload. At 64KB that's a 131KB allocation over the large-object-heap threshold on every comparison, and because the whole payload is decoded before anything is compared, a value differing in its first byte costs the same as one that matches:

64KB, equal            43,121 ns / 131,110 B
64KB, differs at byte 0 57,286 ns / 131,110 B   (raw byte compare: 39 ns)

That's a real cost, and it's the cost you were pointing at.

Why I couldn't take the change as written. RedisValue.GetHashCode() hashes the decoded text — it has to, because that's what equality means on the type. Your Equals compares the encoded bytes. Those look equivalent but aren't, because UTF-8 doesn't round-trip strings holding unpaired surrogates:

"\uD800"  encodes to  EF BF BD   (the same bytes as "\uFFFD")
  byte-domain Equals  -> true
  hash("\uD800") = 241405939 ; hash(blob) = -38304690   -> different buckets

Equal values with different hash codes silently breaks Dictionary and HashSet — no error, just lookups that stop working. And it isn't hypothetical that the shared type's hash matters: I tried the aggressive version myself in #3229 and found it broke a Hashtable keyed by RedisValue and probed by string, which works today. I closed my own PR for the same reason I'm closing yours.

So it wasn't the idea that was wrong, it was the location. Both halves of your PR have now landed, separately:

#3228 removes the allocation without changing what equality means — it decodes in chunks against a stack buffer and stops at the first mismatching chunk. 8× at 64KB, 1092× when values differ early, and zero allocation at every size.
#3230 gives you the byte-domain comparison you actually wanted, as RedisValue.EqualityComparer.Binary. A comparer owns both halves of its own contract, so the byte reading is self-consistent there in a way it can never be on the shared type:
new Dictionary<RedisValue, T>(RedisValue.EqualityComparer.Binary).

It's faster than anything I could do to the type itself, because it never decodes: 64KB equal in 2,155 ns, early mismatch in 15.5 ns, and hashing a 64KB blob in 884 ns against 31,026 ns — 35×.

Your StartsWith(ReadOnlySpan<byte>) bug was real too, and was fixed in 0531cd5; the version there also handles the case where the prefix cut lands between the halves of a surrogate pair.

On your question "why don't you use Marvin in RedisKey?" — still a fair one, still open, and now better informed: hashing runs at roughly 2 GB/s and dominates everything else for large values, with the UTF-8 decode under 4% of it. That's a real decision to make rather than a detail.

Thank you for the push, and sorry it took a detour through three of my own PRs to get there.

[pairbit]

I was happy to help.