Improve MurmurHash string hash memory footprint #5028

Arkatufus · 2021-05-21T16:43:11Z

Edit by @Aaronontheweb : Removed the reference to #5022 since that has other issues still affecting it

Arkatufus · 2021-05-21T16:48:37Z

src/core/Akka/Util/MurmurHash.cs

@@ -202,20 +202,20 @@ public static int StringHash(string s)
        {
            unchecked
            {
-                var sChar = s.ToCharArray();


This is the biggest offending code. MurmurHash is called once for every DataEnvelope that are going to be sent as a gossip. For a system that are trying to gossip thousands of gossips, this can add up quite significantly.

Before

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

Murmur_string_hash 60.87 ns 1.240 ns 2.389 ns 0.0229 - - 96 B

Jenkins_string_hash 221.14 ns 0.767 ns 0.680 ns 0.0153 - - 64 B

Murmur_binary_hash 166.72 ns 2.177 ns 2.036 ns - - - -

Jenkins_binary_hash 370.40 ns 0.709 ns 0.554 ns - - - -

After

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19041.985 (2004/May2020Update/20H1) AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores .NET SDK=5.0.203 [Host] : .NET Core 3.1.15 (CoreCLR 4.700.21.21202, CoreFX 4.700.21.21402), X64 RyuJIT DefaultJob : .NET Core 3.1.15 (CoreCLR 4.700.21.21202, CoreFX 4.700.21.21402), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

Murmur_string_hash 40.39 ns 0.082 ns 0.077 ns - - - -

Jenkins_string_hash 218.54 ns 0.467 ns 0.390 ns 0.0153 - - 64 B

Murmur_binary_hash 166.06 ns 2.224 ns 2.080 ns - - - -

Jenkins_binary_hash 372.00 ns 2.132 ns 1.664 ns - - - -

That is definitely a big improvement.

Arkatufus · 2021-05-21T16:56:02Z

src/contrib/cluster/Akka.DistributedData/VersionVector.cs

@@ -336,7 +378,7 @@ public MultiVersionVector(ImmutableDictionary<UniqueAddress, long> nodeVersions)

        public override bool IsEmpty => Versions.IsEmpty;
        public override int Count => Versions.Count;
-        public override IEnumerator<KeyValuePair<UniqueAddress, long>> VersionEnumerator => Versions.GetEnumerator();


This is the second memory offender. Calling ImmutableDictionary GetEnumerator seemed to create a shallow copy of some of its internal data structure, consuming memory everytime it is called.

Fix by converting this to a Tuple IEnumerator and our own much simpler IEnumerator implementation.

Arkatufus · 2021-05-21T16:58:12Z

There is a third memory offender, the serialization byte array allocation, but I have yet found a good way to fix this.

Aaronontheweb · 2021-05-21T18:00:43Z

@Arkatufus can you do a before and after snapshot on this?

Arkatufus · 2021-05-21T18:02:16Z

Would a dotMemory screenshot be enough?

Aaronontheweb

What do you think about the breaking API change trade-offs?

Aaronontheweb · 2021-05-21T18:09:48Z

src/core/Akka/Actor/Address.cs

@@ -40,7 +40,7 @@ public int Compare(Address x, Address y)
                if (result != 0) return result;
                result = string.CompareOrdinal(x.System, y.System);
                if (result != 0) return result;
-                result = string.CompareOrdinal(x.Host ?? "", y.Host ?? "");
+                result = string.CompareOrdinal(x.Host ?? string.Empty, y.Host ?? string.Empty);


Aaronontheweb · 2021-05-21T18:10:33Z

src/contrib/cluster/Akka.DistributedData/VersionVector.cs

@@ -257,7 +257,7 @@ public SingleVersionVector(UniqueAddress node, long version)

        public override bool IsEmpty => false;
        public override int Count => 1;
-        public override IEnumerator<KeyValuePair<UniqueAddress, long>> VersionEnumerator => new Enumerator(Node, Version);


Is this a breaking API change or is this a subtype of an internal type above?

Yeah, both of these are breaking changes.

I thought about doing this in #4990 - but I decided to avoid the breaking API change by exposing an internal API that should be used for this.

I don't think the risks are that high of this breaking change having a significant negative impact as DData is either primarily used directly or inside of cluster sharding and these APIs aren't called directly... but it's a judgement call.

Arkatufus · 2021-05-21T23:34:17Z

Well, this is highly annoying. I can't reproduce the bug using the original reproduction code anymore, memory consuption stays between 100-200 Mb no matter what I do.

Aaronontheweb · 2021-05-21T23:42:24Z

Is the frequent gossip still occurring?

…

Sent from my iPhone

On May 21, 2021, at 6:34 PM, Gregorius Soedharmo ***@***.***> wrote: Well, this is highly annoying. I can't reproduce the bug using the original reproduction code anymore, memory consuption stays between 100-200 Mb no matter what I do. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

Arkatufus · 2021-05-22T01:00:05Z

Yes, its still occuring, the console is scrolling endlessly, but the memory total didn't go above 200 Mb

Aaronontheweb · 2021-05-24T13:33:42Z

@Arkatufus let's get this PR cleaned up for merge so we can benefit from the improved memory footprint. We can still work on tracking down the gossip issues in another PR.

# Conflicts: # src/contrib/cluster/Akka.DistributedData/VersionVector.cs

Aaronontheweb

LGTM

Reduce memory footprint

f697381

Arkatufus commented May 21, 2021

View reviewed changes

Arkatufus marked this pull request as draft May 21, 2021 17:00

Fix enumerator, off by 1 error

19bd4f3

Aaronontheweb reviewed May 21, 2021

View reviewed changes

Aaronontheweb mentioned this pull request May 21, 2021

Span<char>-ifying ActorPath and Address parsing #5030

Closed

Merge branch 'dev' into FIX_DData_cluster_sharding_pruning

213ea3c

# Conflicts: # src/contrib/cluster/Akka.DistributedData/VersionVector.cs

Arkatufus marked this pull request as ready for review May 24, 2021 15:06

Arkatufus changed the title ~~Resolve Akka.DistributedData DurableDataEnvelope memory leak~~ Improve MurmurHash string hash memory footprint May 24, 2021

Aaronontheweb approved these changes May 24, 2021

View reviewed changes

Aaronontheweb enabled auto-merge (squash) May 24, 2021 16:14

Aaronontheweb added akka-actor akka-ddata perf labels May 24, 2021

Aaronontheweb added this to the 1.4.21 milestone May 24, 2021

Aaronontheweb disabled auto-merge May 24, 2021 21:12

Aaronontheweb merged commit 3f8fa29 into akkadotnet:dev May 24, 2021

Aaronontheweb mentioned this pull request Jun 3, 2021

v1.4.21-beta1 Release Notes #5071

Merged

This was referenced Jun 16, 2021

v1.4.21 final release notes #5095

Merged

v1.4.21 Release #5096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MurmurHash string hash memory footprint #5028

Improve MurmurHash string hash memory footprint #5028

Arkatufus commented May 21, 2021 •

edited by Aaronontheweb

Loading

Arkatufus May 21, 2021

Aaronontheweb May 21, 2021

Aaronontheweb May 21, 2021 •

edited

Loading

Aaronontheweb May 21, 2021

Arkatufus May 21, 2021

Arkatufus commented May 21, 2021

Aaronontheweb commented May 21, 2021

Arkatufus commented May 21, 2021

Aaronontheweb left a comment

Aaronontheweb May 21, 2021

Aaronontheweb May 21, 2021

Aaronontheweb May 21, 2021

Arkatufus commented May 21, 2021

Aaronontheweb commented May 21, 2021 via email

Arkatufus commented May 22, 2021

Aaronontheweb commented May 24, 2021

Aaronontheweb left a comment

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Murmur_string_hash	60.87 ns	1.240 ns	2.389 ns	0.0229	-	-	96 B
Jenkins_string_hash	221.14 ns	0.767 ns	0.680 ns	0.0153	-	-	64 B
Murmur_binary_hash	166.72 ns	2.177 ns	2.036 ns	-	-	-	-
Jenkins_binary_hash	370.40 ns	0.709 ns	0.554 ns	-	-	-	-

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Murmur_string_hash	40.39 ns	0.082 ns	0.077 ns	-	-	-	-
Jenkins_string_hash	218.54 ns	0.467 ns	0.390 ns	0.0153	-	-	64 B
Murmur_binary_hash	166.06 ns	2.224 ns	2.080 ns	-	-	-	-
Jenkins_binary_hash	372.00 ns	2.132 ns	1.664 ns	-	-	-	-

Improve MurmurHash string hash memory footprint #5028

Improve MurmurHash string hash memory footprint #5028

Conversation

Arkatufus commented May 21, 2021 • edited by Aaronontheweb Loading

Arkatufus May 21, 2021

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021

Choose a reason for hiding this comment

Arkatufus May 21, 2021

Choose a reason for hiding this comment

Arkatufus commented May 21, 2021

Aaronontheweb commented May 21, 2021

Arkatufus commented May 21, 2021

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021

Choose a reason for hiding this comment

Aaronontheweb May 21, 2021

Choose a reason for hiding this comment

Arkatufus commented May 21, 2021

Aaronontheweb commented May 21, 2021 via email

Arkatufus commented May 22, 2021

Aaronontheweb commented May 24, 2021

Aaronontheweb left a comment

Choose a reason for hiding this comment

Arkatufus commented May 21, 2021 •

edited by Aaronontheweb

Loading

Aaronontheweb May 21, 2021 •

edited

Loading