Reuse HashHelpers for BinaryFormatter objectholder hashes by ViktorHofer · Pull Request #25509 · dotnet/corefx

ViktorHofer · 2017-11-26T19:19:40Z

Fixes https://github.com/dotnet/corefx/issues/25533
Fixes https://github.com/dotnet/corefx/issues/24902

With #17949 we increased the MaxArrazSize of ObjectHolders. Because of that change our largest prime number 6584983 can now be smaller than the max size of an array. By @danmosemsft suggestion I'm reusing the HashHelpers utility class to utilize larger pre-computed prime numbers and remove the logic for calculating the next optimal prime number.
This also changes the start prime number size as before it was 5 and now it is 3. When I'm off plane I will add benchmarks with performance numbers. A quick test didn't show any noticeable performance regressions.

@Alois-xx I'm marking issue #24902 as fixed. As @stephentoub already mentioned we brought back BinaryFormatter for a smoother transition from .NET Framework to .NET Core and don't intend to optimize it further. If you want to, you can work by yourself on changing the data structure to remove the necessity of the pre-computed prime numbers and therefore the virtual limit. That would require extensive tests to be confident that we don't break or regress existing code.

cc @joperezr

danmoseley · 2017-11-26T19:36:48Z

        {
            if (min < 0)
                throw new ArgumentException(SR.Arg_HTCapacityOverflow);
-            Contract.EndContractBlock();


typo below
// Allow the hashtables to grow to maximum possible size (~2G elements) before encoutering capacity overflow.

danmoseley · 2017-11-26T19:39:49Z

There is another duplicate here:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/src/System/Collections/Hashtable.cs#L1655
It can also be replaced with the CoreFX one that has the larger precomputed list. It has a field SerializationInfoTable that it doesn't use itself that would be moved onto Hashtable proper.

danmoseley · 2017-11-26T19:45:36Z

I see that https://github.com/dotnet/corefx/blob/master/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/ObjectToIdCache.cs could potentially use HashHelpers also. Not suggesting you do this now.

Correction: no it couldn't, as ObjecttoIDCache uses a sequence of primes that are no less than 2x the previous one, whereas HashHelpers uses primes that are no less than 1.2x the previous one. I see GCC uses 2x. Wonder what the history is there.

danmoseley · 2017-11-26T19:58:50Z

Interestingly ObjecttoIdCache.cs will go beyond the largest prime that fits in an int (0x7FEFFFFD) and just use a non prime (0x7FEFFFFF). It says that's the largest possible array size, although I would have thought that was ‭0x7FFFFFFF‬. Maybe there's some overhead. This assumes the hashtables would still work (albeit inefficiently) with a non prime and not eg loop forever.

danmoseley · 2017-11-26T20:23:36Z

 ** 
 ===========================================================*/

-using System;


you can remove the comment above wrapped in ==========

jkotas · 2017-11-26T23:09:38Z

largest possible array size, although I would have thought that was ‭0x7FFFFFFF

The largest possible array size in both .NET Framework and .NET Core is 0X7FEFFFFF (https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/gcallowverylargeobjects-element).

JonHanna · 2017-11-27T00:16:06Z

Largest array size is 0x7FFFFFC7 for 1-byte sized value-type elements and 0X7FEFFFFF otherwise. If you are looking to ObjectToIdCache.GetPrime for guidance it's worth noting that for that case growth is always incremental without deletions, and one at a time. This means that for it a policy of doubling on growth and finding the next available prime can be plotted entirely without waste. For a structure that can have deletions and/or have growth of a predetermined amount other than one at a time the best table would have more intermediate values to catch a wider range of best-fits.

stephentoub · 2017-11-27T12:50:49Z

https://source.dot.net/#System.Private.CoreLib/src/System/Array.cs,499

stephentoub · 2017-11-27T12:52:19Z

  </resheader>
+  <data name="Arg_HTCapacityOverflow" xml:space="preserve">
+    <value>Hashtable's capacity overflowed and went negative. Check load factor, capacity and the current size of the table.</value>
+  </data>


Why is an error in HashHelpers referring to Hashtable? If that were to actually be hit, it would have no relevance to BinaryFormatter.

danmoseley · 2017-11-29T19:36:13Z

@@ -40,24 +30,41 @@ internal static class HashHelpers
            1103, 1327, 1597, 1931, 2333, 2801, 3371, 4049, 4861, 5839, 7013, 8419, 10103, 12143, 14591,


I would add a comment explaining why we don't just grow the table... since apparently it wasn't obvious to us

danmoseley · 2017-11-29T19:37:19Z

    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
+  <data name="Arg_HTCapacityOverflow" xml:space="preserve">
+    <value>Capacity overflowed and went negative.</value>


Something like "Cannot add more than {0} objects." might be more useful/user oriented? They don't care whether we use signed ints.

I didn't address this one as we now should never hit this message unless the array entirely overflows.

stephentoub · 2017-12-05T14:11:10Z

Tests (in particular if this is fixing https://github.com/dotnet/corefx/issues/24902)?

ViktorHofer · 2017-12-05T17:52:15Z

I'm waiting for a decision being made in https://github.com/dotnet/corefx/issues/25533. Precisely speaking, if we should always compute prime numbers and completely get rid of the pre-computed ones. I might benchmark it both ways to get the discussion going again.

jkotas · 2017-12-05T18:24:45Z

if we should always compute prime numbers

I expect that it will be too slow to compute the small prime numbers compared to the table lookup. I think the table make sense for small numbers.

danmoseley · 2017-12-13T21:14:53Z

Overlap with dotnet/coreclr#15453

karelz · 2018-01-18T01:58:29Z

@ViktorHofer if we're blocked, would it make sense to close the PR until it is unblocked?

ViktorHofer · 2018-01-18T12:54:14Z

I don't think we are blocked here. We haven't found a consensus yet. But I agree, the discussion seems to be stall

karelz · 2018-01-18T16:42:48Z

If we can't each consensus in next few days, I would recommend to bring the discussion into an issue/email and close the PR, until we know what to do.

karelz · 2018-01-27T18:16:32Z

Closing, please reopen when consensus is reached.

This reverts commit ddf8ca0, reversing changes made to 0a0ea7f.

ViktorHofer · 2018-03-21T22:22:58Z

Reopening to finally fix the remaining issues that are tagged with 2.1.

joperezr · 2018-03-27T22:31:45Z

@ViktorHofer is this one ready to go now?

ViktorHofer · 2018-03-28T15:48:46Z

Working on this right now.

ViktorHofer · 2018-03-28T17:26:36Z

@stephentoub > Tests (in particular if this is fixing #24902)?

Just added relevant ones.

@stephentoub @jkotas PTAL

ViktorHofer · 2018-03-28T17:53:42Z

Gosh... We have the same implementation in CoreLib shared.

ViktorHofer · 2018-03-28T18:24:44Z

        private Object _syncRoot;

+        private static ConditionalWeakTable<object, SerializationInfo> s_serializationInfoTable;
+        private static ConditionalWeakTable<object, SerializationInfo> SerializationInfoTable => LazyInitializer.EnsureInitialized(ref s_serializationInfoTable);


In CoreLib we use Interlocked.CompareExchange instead. @jkotas any preference here?

internal static ConditionalWeakTable<object, SerializationInfo> SerializationInfoTable { get { if (s_serializationInfoTable == null) Interlocked.CompareExchange(ref s_serializationInfoTable, new ConditionalWeakTable<object, SerializationInfo>(), null); return s_serializationInfoTable; } }

I do not have preference.

LazyInitializer.EnsureInitialized is convenience helper. It looks nicer, but it results into bigger slower code.

thanks :) I'm fine with using LazyInitializer here as the serialization code path isn't highly perf related.

ViktorHofer · 2018-03-28T18:56:27Z

This is failing on x86 Win machines with an OutOfMemoryException.

…efx#25509) * Reuse HashHelpers for BinaryFormatter objectholder hashes * Revert "Merge pull request dotnet/corefx#6203 from SunnyWar/master" This reverts commit dotnet/corefx@ddf8ca0, reversing changes made to dotnet/corefx@0a0ea7f. * Change resource string, make HashTable reuse existing HashHelper * Add comment describing hash number growth * Add hash number growth tests for BinaryFormatter & HashSet * Disable tests on x86 because of OOMs Commit migrated from dotnet/corefx@b6b5982

ViktorHofer added the area-System.Runtime.Extensions label Nov 26, 2017

ViktorHofer self-assigned this Nov 26, 2017

ViktorHofer requested review from danmoseley and stephentoub November 26, 2017 19:19

danmoseley reviewed Nov 26, 2017

View reviewed changes

danmoseley approved these changes Nov 26, 2017

View reviewed changes

stephentoub reviewed Nov 27, 2017

View reviewed changes

ViktorHofer force-pushed the SerializationHolderHash branch 2 times, most recently from 8d4979e to ce441ee Compare November 27, 2017 23:16

danmoseley reviewed Nov 29, 2017

View reviewed changes

karelz closed this Jan 27, 2018

karelz added this to the 2.1.0 milestone Feb 4, 2018

ViktorHofer reopened this Mar 21, 2018

Reuse HashHelpers for BinaryFormatter objectholder hashes

3212106

Revert "Merge pull request dotnet#6203 from SunnyWar/master"

a3d2616

This reverts commit ddf8ca0, reversing changes made to 0a0ea7f.

ViktorHofer force-pushed the SerializationHolderHash branch from ce441ee to eed669a Compare March 21, 2018 22:20

Change resource string, make HashTable reuse existing HashHelper

06daf0e

ViktorHofer force-pushed the SerializationHolderHash branch from eed669a to 06daf0e Compare March 22, 2018 15:02

Add comment describing hash number growth

7bba89b

Add hash number growth tests for BinaryFormatter & HashSet

af85154

ViktorHofer force-pushed the SerializationHolderHash branch from fb7c2c9 to af85154 Compare March 28, 2018 17:29

jkotas approved these changes Mar 28, 2018

View reviewed changes

ViktorHofer commented Mar 28, 2018

View reviewed changes

Disable on x86 because of OOMs

b7c32d2

ViktorHofer merged commit b6b5982 into dotnet:master Mar 28, 2018

ViktorHofer deleted the SerializationHolderHash branch March 28, 2018 22:38

		@@ -40,24 +30,41 @@ internal static class HashHelpers
		1103, 1327, 1597, 1931, 2333, 2801, 3371, 4049, 4861, 5839, 7013, 8419, 10103, 12143, 14591,

Conversation

ViktorHofer commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Nov 26, 2017

Uh oh!

danmoseley commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Nov 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 26, 2017

Uh oh!

JonHanna commented Nov 27, 2017

Uh oh!

stephentoub commented Nov 27, 2017

Uh oh!

stephentoub Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Dec 5, 2017

Uh oh!

jkotas commented Dec 5, 2017

Uh oh!

danmoseley commented Dec 13, 2017

Uh oh!

karelz commented Jan 18, 2018

Uh oh!

ViktorHofer commented Jan 18, 2018

Uh oh!

karelz commented Jan 18, 2018

Uh oh!

karelz commented Jan 27, 2018

Uh oh!

ViktorHofer commented Mar 21, 2018

Uh oh!

joperezr commented Mar 27, 2018

Uh oh!

ViktorHofer commented Mar 28, 2018

Uh oh!

ViktorHofer commented Mar 28, 2018

Uh oh!

ViktorHofer commented Mar 28, 2018

Uh oh!

ViktorHofer Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Mar 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ViktorHofer commented Nov 26, 2017 •

edited

Loading

danmoseley commented Nov 26, 2017 •

edited

Loading

stephentoub Nov 27, 2017 •

edited

Loading

stephentoub commented Dec 5, 2017 •

edited

Loading

ViktorHofer Mar 28, 2018 •

edited

Loading