ADO.NET `IHashPicker` customization API + Orleans v3-compatible `IHashPicker` implementation #9217

vladislav-prishchepa · 2024-11-05T16:18:09Z

references: #9141

In this PR:

added property AdoNetGrainStorageOptions.IHashPicker to support the hash picker customization via DI
added a new IHashPicker implementation Orleans3CompatibleStorageHashPicker for Orleans v3 -> v7+ migration scenarios
added method AdoNetGrainStorageOptions.UseOrleans3CompatibleHasher() for easier Orleans3CompatibleStorageHashPicker configuration

Microsoft Reviewers: Open in CodeFlow

…geOptions

…-only grain id serialization breaking changes handling required

vladislav-prishchepa · 2024-11-05T16:20:14Z

@dotnet-policy-service agree

gfoidl · 2024-11-08T09:29:55Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/JenkinsHash.cs

+    {
+        private static void Mix(ref uint a, ref uint b, ref uint c)
+        {
+            a -= b; a -= c; a ^= (c >> 13);


As a, b, and c are refs, each operation must access the (stack) memory.
When the operations are solely done on locals, then registers can be used.

Cf. sharplab

Out of curiosity I ran a benchmark.

| Method | Mean | Error | StdDev | Ratio | RatioSD | |----------- |----------:|----------:|----------:|------:|--------:| | Default | 27.784 ns | 0.5689 ns | 0.5322 ns | 1.00 | 0.03 | | Suggestion | 6.043 ns | 0.1590 ns | 0.1633 ns | 0.22 | 0.01 |

So roughly 4.5x faster (in the isolated benchmark).

code

using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Running; BenchmarkRunner.Run<Bench>(); public class Bench { private uint _a = 42; private uint _b = 43; private uint _c = 44; [Benchmark(Baseline = true)] public void Default() => Mix(ref _a, ref _b, ref _c); [Benchmark] public void Suggestion() => Mix1(ref _a, ref _b, ref _c); private static void Mix(ref uint a, ref uint b, ref uint c) { a -= b; a -= c; a ^= (c >> 13); b -= c; b -= a; b ^= (a << 8); c -= a; c -= b; c ^= (b >> 13); a -= b; a -= c; a ^= (c >> 12); b -= c; b -= a; b ^= (a << 16); c -= a; c -= b; c ^= (b >> 5); a -= b; a -= c; a ^= (c >> 3); b -= c; b -= a; b ^= (a << 10); c -= a; c -= b; c ^= (b >> 15); } private static void Mix1(ref uint aa, ref uint bb, ref uint cc) { uint a = aa; uint b = bb; uint c = cc; a -= b; a -= c; a ^= (c >> 13); b -= c; b -= a; b ^= (a << 8); c -= a; c -= b; c ^= (b >> 13); a -= b; a -= c; a ^= (c >> 12); b -= c; b -= a; b ^= (a << 16); c -= a; c -= b; c ^= (b >> 5); a -= b; a -= c; a ^= (c >> 3); b -= c; b -= a; b ^= (a << 10); c -= a; c -= b; c ^= (b >> 15); aa = a; bb = b; cc = c; } }

The idea was to keep the implementation of the JenkinsHash exactly the same it was in 3.7.2, without introducing potentially breaking changes.

It's possible to improve it but only with a proper coverage by tests.

I'm not sure that calculation optimization will bring a significant profit. We need to calculate hash only 2 times just before DbCommand execution, ~50ns improvement is unnoticeable in comparison with ADO.NET overhead.

That's true, but think in a different way:

For each operation memory needs to be accessed, so if in L1 read from there, otherwise go the cache hierarchy up to RAM if not found. So we have potential for cache trashing, which can't be shown in micro-benchmarks.

When memory access is minimized, the chance for cache trashing is minimized too.
Here the code change required is trivial, so I'd go with the suggestion -- the algorithm is untouched, just at entry and exit of the method the value are read to local / written back.

OK, pushed the change

gfoidl · 2024-11-08T09:31:57Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/JenkinsHash.cs

+            uint c = 0;
+            int i = 0;
+
+            while (i + 12 <= len)


i gets incremented in the loop. thus the loop-condition nees to be re-evaluated each iteration. Change it to

Suggested change

while (i + 12 <= len)

while (i <= len - 12)

then the loop condition is only evaluated once (and can potentially be kept in a register).

gfoidl · 2024-11-08T09:33:16Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/JenkinsHash.cs

+            while (i + 12 <= len)
+            {
+                a += (uint)data[i++] |
+                    ((uint)data[i++] << 8) |


This kind of indexing results in a lot bound checks. Do we care about these?

Alternative is to use raw-pointers (pinning doesn't harm here).

gfoidl · 2024-11-08T09:40:02Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/JenkinsHash.cs

+            byte[] bytesToHash = Encoding.UTF8.GetBytes(data);
+            return ComputeHash(bytesToHash);


The intermediate allocation for the bye-array can be avoided.

Suggested change

byte[] bytesToHash = Encoding.UTF8.GetBytes(data);

return ComputeHash(bytesToHash);

int maxByteCount = Encoding.UTF8.GetMaxByteCount(data.Length);

Span<byte> buffer = maxByteCount <= 256

? stackalloc byte[256]

: new byte[maxByteCount];

int written = Encoding.UTF8.GetBytes(data, buffer);

return ComputeHash(buffer.Slice(0, written));

(assuming data.Length will most likely be <= 256, otherwise the byte-array fallback could be rented from the array pool instead)

Is it worth to complicate the code here?

removed all JenkinsHash methods except ComputeHash(ReadOnlySpan<byte>) (not used)

gfoidl · 2024-11-08T09:42:07Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleHasher.cs

+        /// <summary>
+        /// <see cref="IHasher.Hash(byte[])"/>.
+        /// </summary>
+        public int Hash(byte[] data)


Suggested change

public int Hash(byte[] data)

public int Hash(ReadOnlySpan<byte> data)

?

It will be a breaking change - IHashPicker interface is a part of the public API.

We can extend IHashPicker with new method Hash(ReadOnlySpan<byte>) with default implementation or add new interface with this method.

It's possible to add method overload just in implementations. May be it's the best option as byte[] data is originally produced by AdoGrainKey.GetHashBytes().

gfoidl · 2024-11-08T09:43:21Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+        /// <summary>
+        /// <see cref="IHasher.Hash(byte[])"/>.
+        /// </summary>
+        public int Hash(byte[] data)


Suggested change

public int Hash(byte[] data)

public int Hash(ReadOnlySpan<byte> data)

I'd go with the ROS on all these arguments.

gfoidl · 2024-11-08T09:44:54Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            var extendedData = new byte[data.Length + 8];
+            data.CopyTo(extendedData, 0);
+            return _innerHasher.Hash(extendedData);


Can extendedData be stack-allocated (or rented from array pool) to avoid that allocation?

~~Yes, if we extend IHashPicker or add a new interface.~~

done (stackalloc + pooled memory fallback)

gfoidl · 2024-11-08T09:46:36Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            // reducing allocations if data is not a grain type
+            if (data.Length >= _grainType.Length && Encoding.UTF8.GetByteCount(_grainType) == data.Length)
+            {
+                var grainTypeBytes = Encoding.UTF8.GetBytes(_grainType);


That's the rare case (when I read the comment correct)?

Otherwise try to avoid the allocation by using stack-space (see comment above for a snippet how to do this).

done (stackalloc + pooled memory fallback)

…nused methods removed

gfoidl · 2024-11-08T12:00:31Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            try
+            {
+                data.AsSpan().CopyTo(buffer);
+                Array.Clear(buffer, data.Length, 8);


Is the Clear needed?
For the stack-alloc path above you can safely assume that the stack-space is zeroed. E.g. when there's [SkipLocalsInit] applied.

I'd combine the two pathes, to avoid the duplicated logic (though it's very little logic here).

yep, we need to clear rented buffer to make sure all these bytes set to zero - rented buffer can contain arbitrary data

Ah yes, I misread the code here. The extra 8 bytes need to be zeroed.
Thanks for the hint!

As the span also needs to be cleared, these two code-pathes should be collapsed (see other comments).

gfoidl · 2024-11-08T12:01:18Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            {
+                var grainTypeBytes = buffer.AsSpan(0, grainTypeByteCount);
+
+                if (!Encoding.UTF8.TryGetBytes(_grainType, grainTypeBytes, out _))


Same here, combine the code paths.

gfoidl · 2024-11-08T12:02:59Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+
+                return grainTypeBytes.SequenceEqual(data);
+            }
+            finally


finally isn't needed. In case of failure the rented array will just be dropped (on the GCed). The array-pool is tolerant to this.
W/o finally there's also room for the JIT to do more optimizations.

The pattern w/ try-finally for the array pool got used some time ago in .NET, but it isn't anymore.
(Same above)

gfoidl · 2024-11-08T12:44:48Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            var extendedLength = data.Length + 8;
+            if (extendedLength <= 256)
+            {
+                Span<byte> extended = stackalloc byte[extendedLength];


Stack-allocated should be to a constant of power of 2, then sliced. It's cheaper.
The extra space (extended) should also be cleared, just to be safe (see SkipLocalsInit).

gfoidl · 2024-11-08T12:46:18Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+            try
+            {
+                data.AsSpan().CopyTo(buffer);
+                Array.Clear(buffer, data.Length, 8);


Ah yes, I misread the code here. The extra 8 bytes need to be zeroed.
Thanks for the hint!

As the span also needs to be cleared, these two code-pathes should be collapsed (see other comments).

gfoidl · 2024-11-08T14:03:40Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs


-            var buffer = ArrayPool<byte>.Shared.Rent(extendedLength);
-            try
+            var buffer = extendedLength switch


This introduces branches that are not necessary. Keep it simple like

int extendedLenght = data.Length + 8; byte[]? bufferFromPool = null; Span<byte> buffer = (extendedLenght <= 256 ? stackalloc byte[256] : bufferFromPool = ArrayPool<byte>.Shared.Rent(extendedLenght) ).Slice(0, extendedLenght); data.AsSpan().CopyTo(buffer); buffer.Slice(data.Length).Clear(); // Hash here if (bufferFromPool is not null) { ArrayPool<byte>.Shared.Return(bufferFromPool); }

but with one more if to assign bufferFromPool

Where's the problem?

That if is cheap.

in code above bufferFromPool is never assigned

Ah sorry, this is from typing the code here within the comments. It's corrected now.

gfoidl · 2024-11-08T14:28:55Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/Orleans3CompatibleStringKeyHasher.cs

+
+            // assuming code below never throws, so calling ArrayPool.Return without try/finally block for JIT optimization
+
+            var buffer = rentedBuffer is not null


Why don't you write it as suggested (i.e. like use almost everywhere with .NET)?

Correct me if I'm wrong.

When we call ArrayPool.Rent and don't call ArrayPool.Return, the buffer we used will be collected by GC. Even if GC will return the array to pool, nobody knows when the GC kicks in. So, further calls to ArrayPool.Return will allocate new arrays (we will quickly deplete the pool by frequent ArrayPool.Rent calls) which will lead to high memory traffic. That's why I return the array explicitly as soon as possible to make it recycled by further ArrayPool.Rent calls.

small repro

[MemoryDiagnoser] public class PoolAllocationBenchmark { [Benchmark] public void Test() { for (var i = 0; i < 1_000_000; i++) { UsePool(); } } [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] private static void UsePool() { var buffer = ArrayPool<byte>.Shared.Rent(1024); Array.Clear(buffer); } }

// * Summary * BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4391/23H2/2023Update/SunValley3) 12th Gen Intel Core i9-12900HX, 1 CPU, 24 logical and 16 physical cores .NET SDK 9.0.100-rc.2.24474.11 [Host] : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX2 DefaultJob : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX2 | Method | Mean | Error | StdDev | Gen0 | Allocated | |------- |---------:|---------:|---------:|-----------:|----------:| | Test | 42.28 ms | 0.596 ms | 0.557 ms | 66750.0000 | 999.45 MB |

I think I've got what you mean (confused by outdated snipped), pushed a change

gfoidl

Looks good now.

Sorry for the confusion with the rented array.

ReubenBond · 2024-11-12T15:58:38Z

src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/AdoNetGrainStorage.cs

@@ -118,7 +118,7 @@ public class AdoNetGrainStorage: IGrainStorage, ILifecycleParticipant<ISiloLifec
        /// <summary>
        /// The hash generator used to hash natural keys, grain ID and grain type to a more narrow index.
        /// </summary>
-        public IStorageHasherPicker HashPicker { get; set; } = new StorageHasherPicker(new[] { new OrleansDefaultHasher() });
+        public IStorageHasherPicker HashPicker { get; set; }


Does this default need to change? It seems that the current behavior should be the default, with the option for the Orleans 3.x compatible

Now we always initialize this property in the constructor, so this initializer is ignored.
But it can be added as a fallback in the constructor to handle cases when value from options is null.

ReubenBond · 2024-11-12T16:01:03Z

src/AdoNet/Orleans.Persistence.AdoNet/Options/AdoNetGrainStorageOptions.cs

+                return;
+
+            // content-aware hashing with different pickers, unable to use standard StorageHasherPicker
+            options.HashPicker = new Orleans3CompatibleStorageHashPicker();


I believe this should be setting the existing default value, new StorageHasherPicker(new[] { new OrleansDefaultHasher() });

Sure, it's a mistake, thanks for checking

vladislav-prishchepa added 4 commits November 1, 2024 15:48

Allow to customize AdoNetGrainStorage.HashPicker via AdoNetGrainStora…

2eb7a2c

…geOptions

Orleans v3-compatible IHasher implementation added

f0b0e57

custom Orleans v3-compatible IHashPicker implementation added: string…

ece6c5b

…-only grain id serialization breaking changes handling required

UseOrleans3CompatibleHasher() method fix

0a78193

Merge branch 'main' into main

cfd2670

gfoidl reviewed Nov 8, 2024

View reviewed changes

Orleans3CompatibleHasher byte[] allocations eliminated, JenkinsHash u…

5493a0c

…nused methods removed

gfoidl reviewed Nov 8, 2024

View reviewed changes

JenkinsHash optimization, Orleans3CompatibleStringKeyHasher refactoring

d82b063

gfoidl reviewed Nov 8, 2024

View reviewed changes

Orleans3CompatibleStringKeyHasher refactoring

21ef966

gfoidl reviewed Nov 8, 2024

View reviewed changes

Orleans3CompatibleStringKeyHasher refactoring

03a33a2

gfoidl approved these changes Nov 8, 2024

View reviewed changes

Merge branch 'main' into main

b6f9b6f

ReubenBond reviewed Nov 12, 2024

View reviewed changes

vladislav-prishchepa added 3 commits November 12, 2024 19:50

default IHashPicker change reverted

d705b2c

IHashPicker configuration comments fix

9b6fc52

AdoNetGrainStorage.HashPicker assignment fallback in ctor

462e8cc

ReubenBond approved these changes Nov 12, 2024

View reviewed changes

ReubenBond merged commit 77cb079 into dotnet:main Nov 12, 2024
16 checks passed

github-actions bot locked and limited conversation to collaborators Dec 13, 2024

		byte[] bytesToHash = Encoding.UTF8.GetBytes(data);
		return ComputeHash(bytesToHash);

-            byte[] bytesToHash = Encoding.UTF8.GetBytes(data);
-            return ComputeHash(bytesToHash);
+            int maxByteCount = Encoding.UTF8.GetMaxByteCount(data.Length);
+            Span<byte> buffer = maxByteCount <= 256
+                ? stackalloc byte[256]
+                : new byte[maxByteCount];
+            int written = Encoding.UTF8.GetBytes(data, buffer);
+            return ComputeHash(buffer.Slice(0, written));

	public int Hash(byte[] data)
	public int Hash(ReadOnlySpan<byte> data)


		// assuming code below never throws, so calling ArrayPool.Return without try/finally block for JIT optimization

		var buffer = rentedBuffer is not null

ADO.NET IHashPicker customization API + Orleans v3-compatible IHashPicker implementation #9217

ADO.NET IHashPicker customization API + Orleans v3-compatible IHashPicker implementation #9217

Conversation

vladislav-prishchepa commented Nov 5, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

vladislav-prishchepa commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladislav-prishchepa Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladislav-prishchepa Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfoidl Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladislav-prishchepa Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfoidl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladislav-prishchepa Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADO.NET `IHashPicker` customization API + Orleans v3-compatible `IHashPicker` implementation #9217

ADO.NET `IHashPicker` customization API + Orleans v3-compatible `IHashPicker` implementation #9217

vladislav-prishchepa commented Nov 5, 2024 •

edited by dotnet-policy-service bot

Loading

vladislav-prishchepa Nov 8, 2024 •

edited

Loading

vladislav-prishchepa Nov 8, 2024 •

edited

Loading

gfoidl Nov 8, 2024 •

edited

Loading

vladislav-prishchepa Nov 8, 2024 •

edited

Loading

vladislav-prishchepa Nov 12, 2024 •

edited

Loading