Removed null checks of buckets and entries in Dictionary and HashSet #48725

TylerBrinkley · 2021-02-24T22:10:31Z

Removed null checks of buckets and entries in Dictionary and HashSet at the cost of some constructor performance. I still need to run perf measurements.

Re-done from dotnet/coreclr#23003

ghost · 2021-02-24T22:10:40Z

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

Removed null checks of buckets and entries in Dictionary and HashSet at the cost of some constructor performance. I still need to run perf measurements.

Author:	TylerBrinkley
Assignees:	-
Labels:	`area-System.Collections`
Milestone:	-

eiriktsarpalis · 2021-02-25T15:03:06Z

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

@@ -51,8 +51,14 @@ public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)

            if (capacity > 0)
            {
-                Initialize(capacity);
+                Initialize(HashHelpers.GetPrime(capacity));


Why has HashHelpers.GetPrime been moved out of the Initialize method? It doesn't like all callsites have been updated to reflect the change.

It was moved out because TrimExcess was already passing in a size computed from HashHelpers.GetPrime so it was doing double work. That is the only callsite I did not update.

eiriktsarpalis · 2021-02-25T15:04:02Z

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

            }
+            else
+            {
+                InitializeEmpty();


This is replaced with eventual calls to Initialize(0). Note however that Initialize(0) will allocate arrays of size 3 instead of 1 in this case. What is the motivation behind this change?

The idea is that using InitializeEmpty will reduce construction costs since it's just initializing with statically initialized arrays. If we called Initialize(0) we'd have to pay those allocation costs upfront even if the dictionary is never written to. It also allows us to maintain current binary serialization logic of not serializing entries if the dictionary hasn't been written to and constructed with a capacity of 0, either explicitly or implicitly.

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs

eiriktsarpalis · 2021-02-25T15:08:23Z

Would you be able to share benchmark results from dotnet/performance using your branch?

…at the cost of some constructor performance.

danmoseley · 2021-03-12T19:55:27Z

Similar approach?
https://github.com/dotnet/corefxlab/blob/e0c8705741ad2b66a1bbc70ff5bf5e02886eca1e/src/Microsoft.Experimental.Collections/Microsoft/Collections/Extensions/DictionarySlim.cs#L33-L37

edit - oh, I see its built on the original backport. I thought it looked familiar!

danmoseley · 2021-03-12T19:55:41Z

cc @benaadams as he works in dictionary periodically..

danmoseley · 2021-03-12T19:59:52Z

@jkotas I remember you had concerns about dotnet/coreclr#22599 but this is a bit different.

benaadams · 2021-03-12T20:04:23Z

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

            }
+            else
+            {
+                uint hashCode = (uint)comparer.GetHashCode(key);
+                int i = GetBucket(hashCode);
+                Entry[] entries = _entries;
+                uint collisionCount = 0;
+                i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
+                do
+                {
+                    // Should be a while loop https://github.com/dotnet/runtime/issues/9422
+                    // Test in if to drop range check for following array access
+                    if ((uint)i >= (uint)entries.Length)
+                    {
+                        goto ReturnNotFound;
+                    }
+
+                    entry = ref entries[i];
+                    if (entry.hashCode == hashCode && comparer.Equals(entry.key, key))
+                    {
+                        goto ReturnFound;
+                    }

-            goto ReturnNotFound;


Still want this goto or its going to throw and exception when something is not found?

It appears this goto statement was only used when the entries was null. Now it shows as unreachable code.

Ah, yes; though its now going to calculate the hashcode and bucket for empty dictionaries?

benaadams · 2021-03-12T20:05:40Z

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

@@ -368,84 +372,49 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte
            }

            ref Entry entry = ref Unsafe.NullRef<Entry>();
-            if (_buckets != null)


Still want an if test here (perhaps .Length > 0 instead, or it will do a lot of work for an empty dictionary?

jkotas · 2021-03-12T20:07:33Z

Yes, I am still concerned about this change. I am interested in the microbenchmark results for this change. I doubt that the improvement is going to be measurable.

On the other hand, this change will increase the static Dictionary footprint and cost to create empty Dictionary in measurable way.

TylerBrinkley · 2021-03-12T20:08:17Z

So I'm trying to run some benchmarks on this and am having difficulty.

I setup two separate copies of this repo on my machine, one for the main branch and one for my branch. I was able to build each of these eventually after several errors but a git clean -dfx helped. Then I followed the guide from the performance repo at https://github.com/dotnet/performance/blob/master/src/benchmarks/micro/README.md#private-runtime-builds and got a bunch of errors saying dotnet 6.0 is not supported by the SDK. I then added the path to main branch repo's dotnet.exe to my environment variables and was able to get the benchmarks to run but they failed with my branch's changes. Am I doing things wrong?

jkotas · 2021-03-12T20:18:50Z

they failed with my branch's changes. Am I doing things wrong?

The functional tests are failing on the change in the CI. I guess that it is also what breaks the perf tests.

benaadams · 2021-03-12T20:31:41Z

The functional tests are failing on the change in the CI.

It will throw InvalidOperationException with "Concurrent Operations Not Supported" when items are not found in the dictionary currently #48725 (comment)

danmoseley · 2021-03-12T20:46:23Z

On the other hand, this change will increase the static Dictionary footprint and cost to create empty Dictionary in measurable way.

Could you explain how it increases the static footprint? I must be missing it.

TylerBrinkley · 2021-03-12T21:23:23Z

It does make an Array.Empty call so that would add to the static footprint per generic type combination.

danmoseley · 2021-03-13T02:40:20Z

Ah yes.

TylerBrinkley · 2021-03-15T01:16:57Z

Here's the benchmarks I ran. It looks like there's just marginal improvements to most tests if any. The biggest surprise is the improvement for the CtorFromCollection<Int32> execution. I don't understand the vast improvement there.

BenchmarkDotNet=v0.12.1.1466-nightly, OS=Windows 10.0.19041.867 (2004/May2020Update/20H1)
AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.100-preview.2.21155.3
  [Host]     : .NET 6.0.0 (6.0.21.15406), X64 RyuJIT
  Job-CSABXJ : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
  Job-DDDSNE : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable  IterationTime=250.0000 ms  
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1

Type	Method	Job	Mean	Error	StdDev	Ratio
DictionarySequentialKeys	ContainsValue_17_Int_Int	Branch	4.108 ns	0.0033 ns	0.0031 ns	0.98
DictionarySequentialKeys	ContainsValue_17_Int_Int	Main	4.211 ns	0.0068 ns	0.0060 ns	1.00
DictionarySequentialKeys	ContainsKey_17_Int_32ByteValue	Branch	4.130 ns	0.0136 ns	0.0127 ns	0.97
DictionarySequentialKeys	ContainsKey_17_Int_32ByteValue	Main	4.269 ns	0.0198 ns	0.0186 ns	1.00
DictionarySequentialKeys	ContainsKey_17_Int_32ByteRefsValue	Branch	4.469 ns	0.0066 ns	0.0058 ns	1.00
DictionarySequentialKeys	ContainsKey_17_Int_32ByteRefsValue	Main	4.455 ns	0.0026 ns	0.0022 ns	1.00
CtorDefaultSize<Int32>	Dictionary	Branch	13.007 ns	0.0562 ns	0.0526 ns	1.91
CtorDefaultSize<Int32>	Dictionary	Main	6.826 ns	0.0561 ns	0.0525 ns	1.00
CtorDefaultSize<String>	Dictionary	Branch	21.200 ns	0.0470 ns	0.0367 ns	1.73
CtorDefaultSize<String>	Dictionary	Main	12.224 ns	0.0351 ns	0.0311 ns	1.00
DictionarySequentialKeys	ContainsValue_3k_Int_Int	Branch	4.029 ns	0.0012 ns	0.0012 ns	0.97
DictionarySequentialKeys	ContainsValue_3k_Int_Int	Main	4.171 ns	0.0045 ns	0.0043 ns	1.00
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteValue	Branch	4.040 ns	0.0014 ns	0.0012 ns	0.96
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteValue	Main	4.225 ns	0.0014 ns	0.0012 ns	1.00
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteRefsValue	Branch	4.406 ns	0.0009 ns	0.0008 ns	1.03
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteRefsValue	Main	4.296 ns	0.0014 ns	0.0012 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_Int	Branch	4.570 ns	0.0023 ns	0.0020 ns	0.95
DictionarySequentialKeys	TryGetValue_17_Int_Int	Main	4.817 ns	0.0069 ns	0.0065 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_32ByteValue	Branch	4.646 ns	0.0088 ns	0.0082 ns	0.95
DictionarySequentialKeys	TryGetValue_17_Int_32ByteValue	Main	4.876 ns	0.0090 ns	0.0085 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_32ByteRefsValue	Branch	4.981 ns	0.0052 ns	0.0049 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_32ByteRefsValue	Main	4.994 ns	0.0055 ns	0.0052 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_Int	Branch	4.238 ns	0.0009 ns	0.0007 ns	0.96
DictionarySequentialKeys	TryGetValue_3k_Int_Int	Main	4.420 ns	0.0043 ns	0.0034 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteValue	Branch	4.299 ns	0.0017 ns	0.0016 ns	0.95
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteValue	Main	4.514 ns	0.0015 ns	0.0013 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteRefsValue	Branch	4.712 ns	0.0020 ns	0.0016 ns	1.02
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteRefsValue	Main	4.619 ns	0.0021 ns	0.0019 ns	1.00
TryAddDefaultSize<Int32>	Dictionary	Branch	8,143.072 ns	169.7940 ns	195.5350 ns	0.98
TryAddDefaultSize<Int32>	Dictionary	Main	8,299.819 ns	144.7992 ns	135.4453 ns	1.00
TryAddDefaultSize<String>	Dictionary	Branch	14,292.531 ns	73.2516 ns	61.1684 ns	0.98
TryAddDefaultSize<String>	Dictionary	Main	14,570.091 ns	76.7112 ns	71.7557 ns	1.00
TryAddGiventSize<Int32>	Dictionary	Branch	4,374.550 ns	92.6551 ns	102.9859 ns	0.94
TryAddGiventSize<Int32>	Dictionary	Main	4,670.566 ns	34.9035 ns	30.9411 ns	1.00
TryAddGiventSize<String>	Dictionary	Branch	10,375.252 ns	44.6383 ns	39.5707 ns	0.97
TryAddGiventSize<String>	Dictionary	Main	10,749.444 ns	58.5008 ns	54.7217 ns	1.00
ContainsKeyFalse<Int32, Int32>	Dictionary	Branch	2,345.344 ns	17.1971 ns	16.0862 ns	1.02
ContainsKeyFalse<Int32, Int32>	Dictionary	Main	2,309.777 ns	8.3239 ns	7.7862 ns	1.00
ContainsKeyFalse<String, String>	Dictionary	Branch	7,443.701 ns	43.9892 ns	38.9953 ns	0.99
ContainsKeyFalse<String, String>	Dictionary	Main	7,513.919 ns	70.5581 ns	66.0001 ns	1.00
ContainsKeyTrue<Int32, Int32>	Dictionary	Branch	2,607.768 ns	1.8513 ns	1.4454 ns	0.99
ContainsKeyTrue<Int32, Int32>	Dictionary	Main	2,639.622 ns	1.3777 ns	1.2887 ns	1.00
ContainsKeyTrue<String, String>	Dictionary	Branch	9,227.279 ns	30.6339 ns	28.6550 ns	1.02
ContainsKeyTrue<String, String>	Dictionary	Main	9,085.092 ns	44.3275 ns	41.4640 ns	1.00
TryGetValueFalse<Int32, Int32>	Dictionary	Branch	2,507.062 ns	19.0514 ns	17.8207 ns	1.03
TryGetValueFalse<Int32, Int32>	Dictionary	Main	2,423.156 ns	20.6920 ns	19.3554 ns	1.00
TryGetValueFalse<String, String>	Dictionary	Branch	7,520.683 ns	37.4133 ns	34.9964 ns	0.98
TryGetValueFalse<String, String>	Dictionary	Main	7,656.863 ns	45.8314 ns	42.8707 ns	1.00
TryGetValueTrue<Int32, Int32>	Dictionary	Branch	2,662.313 ns	6.8007 ns	6.0286 ns	0.97
TryGetValueTrue<Int32, Int32>	Dictionary	Main	2,742.693 ns	10.7229 ns	10.0302 ns	1.00
TryGetValueTrue<String, String>	Dictionary	Branch	9,902.599 ns	54.6011 ns	51.0739 ns	0.97
TryGetValueTrue<String, String>	Dictionary	Main	10,165.563 ns	37.1866 ns	34.7844 ns	1.00
AddGivenSize<Int32>	Dictionary	Branch	4,543.221 ns	93.8050 ns	108.0259 ns	0.96
AddGivenSize<Int32>	Dictionary	Main	4,748.518 ns	82.7117 ns	77.3686 ns	1.00
AddGivenSize<String>	Dictionary	Branch	10,550.591 ns	94.7035 ns	88.5857 ns	0.98
AddGivenSize<String>	Dictionary	Main	10,798.521 ns	54.0674 ns	47.9293 ns	1.00
CreateAddAndRemove<Int32>	Dictionary	Branch	11,060.153 ns	199.6242 ns	186.7286 ns	0.95
CreateAddAndRemove<Int32>	Dictionary	Main	11,611.011 ns	75.5073 ns	66.9352 ns	1.00
CreateAddAndRemove<String>	Dictionary	Branch	25,419.134 ns	86.3334 ns	80.7563 ns	0.98
CreateAddAndRemove<String>	Dictionary	Main	25,995.997 ns	50.3617 ns	44.6443 ns	1.00
CtorFromCollection<Int32>	Dictionary	Branch	1,660.009 ns	22.3808 ns	19.8400 ns	0.60
CtorFromCollection<Int32>	Dictionary	Main	2,801.272 ns	110.0337 ns	126.7150 ns	1.00
CtorFromCollection<String>	Dictionary	Branch	3,579.352 ns	4.3794 ns	3.8822 ns	1.00
CtorFromCollection<String>	Dictionary	Main	3,570.357 ns	6.6729 ns	5.5722 ns	1.00
CtorGivenSize<Int32>	Dictionary	Branch	501.198 ns	1.4160 ns	1.3246 ns	1.01
CtorGivenSize<Int32>	Dictionary	Main	496.821 ns	1.5154 ns	1.1831 ns	1.00
CtorGivenSize<String>	Dictionary	Branch	702.838 ns	2.9709 ns	2.4808 ns	1.04
CtorGivenSize<String>	Dictionary	Main	676.945 ns	2.0242 ns	1.6903 ns	1.00
IndexerSet<Int32>	Dictionary	Branch	3,249.942 ns	8.3109 ns	7.7740 ns	0.97
IndexerSet<Int32>	Dictionary	Main	3,359.845 ns	8.3412 ns	7.3943 ns	1.00
IndexerSet<String>	Dictionary	Branch	10,595.456 ns	50.4108 ns	44.6879 ns	1.00
IndexerSet<String>	Dictionary	Main	10,575.159 ns	36.4121 ns	32.2784 ns	1.00
CreateAddAndClear<Int32>	Dictionary	Branch	7,466.638 ns	99.1499 ns	92.7449 ns	0.90
CreateAddAndClear<Int32>	Dictionary	Main	8,320.824 ns	127.0217 ns	112.6015 ns	1.00
CreateAddAndClear<String>	Dictionary	Branch	15,598.358 ns	62.5055 ns	58.4677 ns	1.04
CreateAddAndClear<String>	Dictionary	Main	15,020.706 ns	50.0175 ns	46.7864 ns	1.00
IterateForEach<Int32>	Dictionary	Branch	1,285.473 ns	0.8596 ns	0.8041 ns	1.00
IterateForEach<Int32>	Dictionary	Main	1,287.054 ns	0.7550 ns	0.7062 ns	1.00
IterateForEach<String>	Dictionary	Branch	2,846.297 ns	0.7700 ns	0.6825 ns	1.05
IterateForEach<String>	Dictionary	Main	2,721.905 ns	1.2846 ns	1.0030 ns	1.00
Perf_Dictionary	ContainsValue	Branch	4,136,881.138 ns	3,030.5771 ns	2,686.5277 ns	1.00
Perf_Dictionary	ContainsValue	Main	4,138,700.625 ns	4,481.9476 ns	4,192.4166 ns	1.00

danmoseley · 2021-03-15T03:32:04Z

fwiw the geo mean of the "branch" ratios above is 1.002. Exclude constructors, which are presumably more rarely called, and it's 0.98

…pty array instead of using Array.Empty<Entry>() since Entry is internal and to avoid the extra generic instantiation

TylerBrinkley · 2021-03-15T21:43:47Z

I just ran it again and got these results. What's interesting is that CtorFromCollection<int> actually went up to 1.21 of base vs 0.60 of base before. For some reason I think that benchmark with the struct generic type argument is flaky with my machine. Including all of these metrics the geo mean of the "branch" ratios is 1.003 but if you exclude the constructors, which are presumably more rarely called as well as that flaky benchmark mentioned above it's 0.965. I'd love to see others do this benchmarking on their machines as well to confirm my findings. I'm using the following benchmark command and filtering out the concurrent dictionary benchmarks.

dotnet run -c Release -f net6.0 --filter System.Collections*.Dictionary* *.Perf_Dictionary.* --join --coreRun "PathToMainCoreRunExe" "PathToBranchCoreRunExe"

BenchmarkDotNet=v0.12.1.1466-nightly, OS=Windows 10.0.19041.867 (2004/May2020Update/20H1)
AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.100-preview.2.21155.3
  [Host]     : .NET 6.0.0 (6.0.21.15406), X64 RyuJIT
  Job-QHDUBI : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
  Job-CLVDGL : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable  IterationTime=250.0000 ms  
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1

Type	Method	Job	Mean	Error	StdDev	Ratio
DictionarySequentialKeys	ContainsValue_17_Int_Int	Branch	4.102 ns	0.0050 ns	0.0046 ns	0.96
DictionarySequentialKeys	ContainsValue_17_Int_Int	Main	4.285 ns	0.0034 ns	0.0032 ns	1.00
DictionarySequentialKeys	ContainsKey_17_Int_32ByteValue	Branch	4.123 ns	0.0037 ns	0.0035 ns	0.95
DictionarySequentialKeys	ContainsKey_17_Int_32ByteValue	Main	4.362 ns	0.0086 ns	0.0080 ns	1.00
DictionarySequentialKeys	ContainsKey_17_Int_32ByteRefsValue	Branch	4.439 ns	0.0104 ns	0.0092 ns	0.99
DictionarySequentialKeys	ContainsKey_17_Int_32ByteRefsValue	Main	4.478 ns	0.0028 ns	0.0027 ns	1.00
CtorDefaultSize<Int32>	Dictionary	Branch	10.083 ns	0.0110 ns	0.0098 ns	1.75
CtorDefaultSize<Int32>	Dictionary	Main	5.771 ns	0.0048 ns	0.0040 ns	1.00
CtorDefaultSize<String>	Dictionary	Branch	18.119 ns	0.0750 ns	0.0702 ns	1.48
CtorDefaultSize<String>	Dictionary	Main	12.266 ns	0.0387 ns	0.0323 ns	1.00
DictionarySequentialKeys	ContainsValue_3k_Int_Int	Branch	4.025 ns	0.0008 ns	0.0007 ns	0.97
DictionarySequentialKeys	ContainsValue_3k_Int_Int	Main	4.169 ns	0.0046 ns	0.0043 ns	1.00
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteValue	Branch	4.035 ns	0.0009 ns	0.0008 ns	0.95
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteValue	Main	4.229 ns	0.0022 ns	0.0017 ns	1.00
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteRefsValue	Branch	4.403 ns	0.0013 ns	0.0011 ns	1.03
DictionarySequentialKeys	ContainsKey_3k_Int_32ByteRefsValue	Main	4.283 ns	0.0032 ns	0.0028 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_Int	Branch	4.679 ns	0.0049 ns	0.0046 ns	0.94
DictionarySequentialKeys	TryGetValue_17_Int_Int	Main	4.954 ns	0.0122 ns	0.0114 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_32ByteValue	Branch	4.765 ns	0.0059 ns	0.0055 ns	0.95
DictionarySequentialKeys	TryGetValue_17_Int_32ByteValue	Main	4.994 ns	0.0093 ns	0.0087 ns	1.00
DictionarySequentialKeys	TryGetValue_17_Int_32ByteRefsValue	Branch	4.931 ns	0.0150 ns	0.0140 ns	0.97
DictionarySequentialKeys	TryGetValue_17_Int_32ByteRefsValue	Main	5.084 ns	0.0044 ns	0.0041 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_Int	Branch	4.238 ns	0.0014 ns	0.0013 ns	0.96
DictionarySequentialKeys	TryGetValue_3k_Int_Int	Main	4.419 ns	0.0014 ns	0.0013 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteValue	Branch	4.295 ns	0.0017 ns	0.0016 ns	0.95
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteValue	Main	4.513 ns	0.0027 ns	0.0025 ns	1.00
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteRefsValue	Branch	4.700 ns	0.0054 ns	0.0050 ns	1.02
DictionarySequentialKeys	TryGetValue_3k_Int_32ByteRefsValue	Main	4.608 ns	0.0014 ns	0.0012 ns	1.00
TryAddDefaultSize<Int32>	Dictionary	Branch	7,263.644 ns	146.3340 ns	168.5184 ns	0.83
TryAddDefaultSize<Int32>	Dictionary	Main	8,776.501 ns	239.8210 ns	276.1782 ns	1.00
TryAddDefaultSize<String>	Dictionary	Branch	13,411.392 ns	81.0619 ns	63.2878 ns	0.97
TryAddDefaultSize<String>	Dictionary	Main	13,848.144 ns	38.7440 ns	36.2412 ns	1.00
TryAddGiventSize<Int32>	Dictionary	Branch	4,921.942 ns	119.2734 ns	137.3554 ns	0.95
TryAddGiventSize<Int32>	Dictionary	Main	5,202.506 ns	106.4715 ns	122.6127 ns	1.00
TryAddGiventSize<String>	Dictionary	Branch	9,956.395 ns	27.6241 ns	24.4880 ns	0.97
TryAddGiventSize<String>	Dictionary	Main	10,313.150 ns	44.9056 ns	42.0047 ns	1.00
ContainsKeyFalse<Int32, Int32>	Dictionary	Branch	2,217.103 ns	3.0186 ns	2.3567 ns	0.98
ContainsKeyFalse<Int32, Int32>	Dictionary	Main	2,263.233 ns	15.5792 ns	13.0093 ns	1.00
ContainsKeyFalse<String, String>	Dictionary	Branch	7,210.737 ns	31.9895 ns	29.9230 ns	0.97
ContainsKeyFalse<String, String>	Dictionary	Main	7,446.576 ns	39.3359 ns	32.8472 ns	1.00
ContainsKeyTrue<Int32, Int32>	Dictionary	Branch	2,579.523 ns	0.7442 ns	0.5811 ns	0.97
ContainsKeyTrue<Int32, Int32>	Dictionary	Main	2,651.400 ns	9.0238 ns	8.4409 ns	1.00
ContainsKeyTrue<String, String>	Dictionary	Branch	9,073.909 ns	29.8070 ns	26.4232 ns	0.98
ContainsKeyTrue<String, String>	Dictionary	Main	9,290.689 ns	55.1202 ns	48.8626 ns	1.00
TryGetValueFalse<Int32, Int32>	Dictionary	Branch	2,321.078 ns	25.7215 ns	22.8014 ns	0.97
TryGetValueFalse<Int32, Int32>	Dictionary	Main	2,400.712 ns	12.1404 ns	10.1378 ns	1.00
TryGetValueFalse<String, String>	Dictionary	Branch	7,528.819 ns	23.3484 ns	20.6977 ns	0.98
TryGetValueFalse<String, String>	Dictionary	Main	7,681.400 ns	53.7661 ns	50.2929 ns	1.00
TryGetValueTrue<Int32, Int32>	Dictionary	Branch	2,643.553 ns	5.1388 ns	4.8069 ns	0.98
TryGetValueTrue<Int32, Int32>	Dictionary	Main	2,710.145 ns	2.9644 ns	2.4754 ns	1.00
TryGetValueTrue<String, String>	Dictionary	Branch	9,824.246 ns	35.1055 ns	29.3147 ns	0.96
TryGetValueTrue<String, String>	Dictionary	Main	10,278.014 ns	57.7226 ns	51.1696 ns	1.00
AddGivenSize<Int32>	Dictionary	Branch	4,275.862 ns	223.9203 ns	257.8669 ns	0.85
AddGivenSize<Int32>	Dictionary	Main	5,013.815 ns	140.0832 ns	155.7021 ns	1.00
AddGivenSize<String>	Dictionary	Branch	10,074.981 ns	53.8518 ns	47.7383 ns	0.96
AddGivenSize<String>	Dictionary	Main	10,488.054 ns	47.8958 ns	44.8018 ns	1.00
CreateAddAndRemove<Int32>	Dictionary	Branch	10,630.563 ns	269.5331 ns	299.5853 ns	0.97
CreateAddAndRemove<Int32>	Dictionary	Main	11,095.175 ns	112.1723 ns	99.4378 ns	1.00
CreateAddAndRemove<String>	Dictionary	Branch	25,015.662 ns	309.5380 ns	289.5420 ns	1.00
CreateAddAndRemove<String>	Dictionary	Main	25,048.459 ns	76.2497 ns	67.5934 ns	1.00
CtorFromCollection<Int32>	Dictionary	Branch	2,712.915 ns	51.1988 ns	50.2840 ns	1.22
CtorFromCollection<Int32>	Dictionary	Main	2,264.890 ns	140.7924 ns	162.1368 ns	1.00
CtorFromCollection<String>	Dictionary	Branch	3,334.400 ns	2.1623 ns	2.0226 ns	1.00
CtorFromCollection<String>	Dictionary	Main	3,342.850 ns	15.1155 ns	13.3995 ns	1.00
CtorGivenSize<Int32>	Dictionary	Branch	314.231 ns	0.4862 ns	0.4548 ns	1.01
CtorGivenSize<Int32>	Dictionary	Main	310.145 ns	1.6681 ns	1.3929 ns	1.00
CtorGivenSize<String>	Dictionary	Branch	446.108 ns	0.9408 ns	0.7856 ns	1.01
CtorGivenSize<String>	Dictionary	Main	440.034 ns	0.4359 ns	0.4077 ns	1.00
IndexerSet<Int32>	Dictionary	Branch	3,263.633 ns	14.9025 ns	11.6349 ns	0.96
IndexerSet<Int32>	Dictionary	Main	3,397.037 ns	14.1503 ns	11.8162 ns	1.00
IndexerSet<String>	Dictionary	Branch	10,269.060 ns	34.0259 ns	31.8279 ns	0.96
IndexerSet<String>	Dictionary	Main	10,650.273 ns	45.7767 ns	40.5799 ns	1.00
CreateAddAndClear<Int32>	Dictionary	Branch	7,073.139 ns	141.0031 ns	131.8944 ns	0.92
CreateAddAndClear<Int32>	Dictionary	Main	7,681.506 ns	103.6304 ns	91.8656 ns	1.00
CreateAddAndClear<String>	Dictionary	Branch	13,881.565 ns	65.0559 ns	60.8533 ns	0.98
CreateAddAndClear<String>	Dictionary	Main	14,164.414 ns	77.4045 ns	72.4042 ns	1.00
IterateForEach<Int32>	Dictionary	Branch	1,287.018 ns	1.1010 ns	0.9760 ns	1.00
IterateForEach<Int32>	Dictionary	Main	1,289.253 ns	2.1942 ns	2.0525 ns	1.00
IterateForEach<String>	Dictionary	Branch	2,721.216 ns	2.2122 ns	2.0693 ns	0.91
IterateForEach<String>	Dictionary	Main	2,974.326 ns	1.5073 ns	1.2586 ns	1.00
Perf_Dictionary	ContainsValue	Branch	4,146,523.125 ns	5,036.5274 ns	4,711.1709 ns	1.00
Perf_Dictionary	ContainsValue	Main	4,141,966.518 ns	5,643.3177 ns	5,002.6541 ns	1.00

ericstj · 2021-04-19T16:25:48Z

Reviewers can we have a final look and sign off or provide feedback? Thanks!

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs

jkotas · 2021-04-19T16:39:56Z

src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs

@@ -343,10 +343,10 @@ public MethodInfo GetArrayMethod(Type arrayClass, string methodName, CallingConv
        public EnumBuilder DefineEnum(string name, TypeAttributes visibility, Type underlyingType)
        {
            ITypeIdentifier ident = TypeIdentifiers.FromInternal(name);
-            if (name_cache.ContainsKey(ident))
+            if (name != null && name_cache.ContainsKey(ident))


I suspect that we will likely see more instances of breaks like this one in the code out there.

Do we need to file a breaking change notification for this change?

So you think we should file a breaking change notification to indicate that the GetHashCode method will be called on a key provided to an initially empty Dictionary whereas before it did not?

I would like to hear what others think about this issue.

cc @marklio @GrabYourPitchforks for thoughts.

It's not clear to me what the break was. Was there a bug in the implementation of <some_concrete_type>.GetHashCode where it was throwing an exception when called? Normally one would expect this method to be called as part of dictionary / hashset processing.

@marklio If I remember correctly, we have done analysis on the capacity distribution for common collections in real apps. How common are empty dictionaries?

Also, it would take just one extra if-check to mitigate this issue: https://github.com/dotnet/runtime/pull/48725/files#r593419736

@TylerBrinkley do you know how reintroducing a check here will affect perf results? I'm guessing that an "is empty" check will cost about the same as the original "is null" check, but it'd be good to confirm.

I just saw this thread. I'll see if we had information on empty dictionaries today.

The last analysis we did of dictionaries was back in 2018, where we were interested in the usage of types as keys to help inform what investments we should make in dictionaries. Data retention policies mean that the underlying study data is long gone, so I can't provide a bunch of backing data, but from summaries, it did indicate that empty dictionaries are common (although we can't tell what operations/frequency are performed on them, or how long they stay empty). There aren't resources to do another study at this time. My guess based on available data here is that operations on empty dictionaries are pretty common. IMO, I tend to agree with @GrabYourPitchforks that perf-sensitive operations on "chronically empty" dictionaries probably aren't common if they were important enough to measure, and profiling would likely shine a pretty direct light on the culprit if it was important enough to measure (and differing perf characteristics are pretty expected in new versions). The functional breaks associated with this change (which we know exist due to the presence of such fixes in the PR) are likely unexpected and pretty hard to reason about if people hit them, and they may not be in a position to be able to address them if they don't own the types involved. I'd say if an easy acceptable mitigation exists, we should take it.

danmoseley · 2021-04-19T17:38:03Z

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs(17,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace

TylerBrinkley · 2021-04-19T17:44:24Z

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs(17,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace

That'll teach me to edit directly within GitHub again.

TylerBrinkley · 2021-04-20T20:42:14Z

/azp run runtime

azure-pipelines · 2021-04-20T20:42:19Z

Commenter does not have sufficient privileges for PR 48725 in repo dotnet/runtime

ericstj · 2021-04-21T00:37:04Z

/azp run runtime

azure-pipelines · 2021-04-21T00:37:31Z

Azure Pipelines successfully started running 1 pipeline(s).

danmoseley · 2021-05-17T15:43:06Z

@TylerBrinkley do you have time to fix the merge conflicts, and address feedback above?

ericstj · 2021-06-07T20:54:33Z

@TylerBrinkley could you please let us know if you're planning to get back to this?

ericstj · 2021-06-08T15:35:17Z

Thanks Tyler, reviewers, can we have another look?

jkotas · 2021-06-08T15:47:59Z

and address feedback above?

I think we should add back the early out for empty table. I do not think that the potential functional and performance regressions are worth it.

TylerBrinkley · 2021-06-08T16:15:11Z

Sorry, my computer's cpu cooler died on me so I can't do a comparison with adding the early empty collection check back in right now. If I had to guess I think the performance would be similar to the prior null check and thus would make this change not worthwhile. I think to justify the performance regression with empty collections we would need to know better how often an empty collection has lookups performed as opposed to collections with items. When I get my computer up and running again I can calculate that breakeven point where when a certain percentage of calls are made with items compared to empty collections, the net time throughput is the same.

In regards to the functional difference, I don't think users should find it surprising that the GetHashCode method on the key they provide will be called when doing a lookup. The fact that it didn't before for an empty collection was simply an implementation detail. If a user relies on that then they must already know their collection is empty and have no need to make the lookup in the first place.

danmoseley · 2021-07-12T23:08:00Z

@TylerBrinkley how is your computer 😄 Just going through old PR's.

eiriktsarpalis · 2021-07-19T10:25:20Z

@jkotas @danmoseley should we maybe close this for now? It seems unlikely it would make it in time for .NET 6.

stephentoub · 2021-08-16T02:26:13Z

Per the discussion, I'm going to close this. Thanks for all the great effort here.

TylerBrinkley · 2021-08-16T13:09:01Z

Sorry, I was very busy with other things and was encountering an unexpected regression I was trying to understand. As time allows I will look more into this for .NET 7.0.

dotnet-issue-labeler bot added the area-System.Collections label Feb 24, 2021

eiriktsarpalis reviewed Feb 25, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs Outdated Show resolved Hide resolved

Base automatically changed from master to main March 1, 2021 09:08

Removed null checks of buckets and entries in Dictionary and HashSet …

b6f24a1

…at the cost of some constructor performance.

TylerBrinkley force-pushed the dictionary-test branch from 1b50167 to b6f24a1 Compare March 12, 2021 17:15

benaadams suggested changes Mar 12, 2021

View reviewed changes

Fix divide by zero bug

8efb22d

TylerBrinkley added 2 commits March 15, 2021 09:26

Cache fast mod multiplier for divisor of 1 and internally cache an em…

88c2fd6

…pty array instead of using Array.Empty<Entry>() since Entry is internal and to avoid the extra generic instantiation

Merge from main

f66a439

jkotas reviewed Apr 19, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs Show resolved Hide resolved

jkotas reviewed Apr 19, 2021

View reviewed changes

Update HashHelpers.cs

6f80ec6

Remove trailing whitespace

c750af8

runfoapp bot mentioned this pull request Apr 21, 2021

System.Tests.GCTests.LatencyRoundtrips failing in CI #51635

Closed

ericstj assigned eiriktsarpalis May 17, 2021

Merge branch 'main' into dictionary-test

c66581e

ericstj requested review from jkotas and eiriktsarpalis June 8, 2021 15:35

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

karelz mentioned this pull request Jul 28, 2021

System.Net.Security test run killed on Mono Linux #56478

Closed

eiriktsarpalis added this to the 7.0.0 milestone Aug 3, 2021

stephentoub closed this Aug 16, 2021

ghost locked as resolved and limited conversation to collaborators Sep 15, 2021

Removed null checks of buckets and entries in Dictionary and HashSet #48725

Removed null checks of buckets and entries in Dictionary and HashSet #48725

Conversation

TylerBrinkley commented Feb 24, 2021 • edited Loading

ghost commented Feb 24, 2021

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

eiriktsarpalis commented Feb 25, 2021

danmoseley commented Mar 12, 2021 • edited Loading

danmoseley commented Mar 12, 2021

danmoseley commented Mar 12, 2021

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

jkotas commented Mar 12, 2021

TylerBrinkley commented Mar 12, 2021

jkotas commented Mar 12, 2021

benaadams commented Mar 12, 2021 • edited Loading

danmoseley commented Mar 12, 2021

TylerBrinkley commented Mar 12, 2021

danmoseley commented Mar 13, 2021

TylerBrinkley commented Mar 15, 2021

danmoseley commented Mar 15, 2021

TylerBrinkley commented Mar 15, 2021

ericstj commented Apr 19, 2021

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

jkotas Apr 21, 2021 • 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

danmoseley commented Apr 19, 2021

TylerBrinkley commented Apr 19, 2021

TylerBrinkley commented Apr 20, 2021

azure-pipelines bot commented Apr 20, 2021

ericstj commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

danmoseley commented May 17, 2021

ericstj commented Jun 7, 2021

ericstj commented Jun 8, 2021

jkotas commented Jun 8, 2021 • edited Loading

TylerBrinkley commented Jun 8, 2021

danmoseley commented Jul 12, 2021

eiriktsarpalis commented Jul 19, 2021

stephentoub commented Aug 16, 2021

TylerBrinkley commented Aug 16, 2021

TylerBrinkley commented Feb 24, 2021 •

edited

Loading

danmoseley commented Mar 12, 2021 •

edited

Loading

benaadams commented Mar 12, 2021 •

edited

Loading

jkotas Apr 21, 2021 •

edited

Loading

jkotas commented Jun 8, 2021 •

edited

Loading