Improve Dictionary TryGetValue size/perfomance

[benaadams]

From #27149 without the hashcode-to-bucket mapping changes

• Avoid second bounds check; by passing a ref rather than an index to TryGetValue/indexer get methods; so a second dip back to the backing array is not needed.
• Return null ref from FindValue so ContainsKey doesn't pay a struct copy cost when using same method.
• Rearrange FindValue return block to be a straighter flow for the common path of found item
• Rearrange Entry struct to put hashCode first rather than next as it is accessed first which should work with prefetching better.
• move collisionCount++ before collisionCount test so the Jit could micro-fuse them (though doesn't currently https://github.com/dotnet/coreclr/issues/7566)

Size improvements

Total bytes of diff: -1647 (-0.05% of base) diff is an improvement. Total byte diff includes -118 bytes from reconciling methods Base had 4 unique methods, 3935 unique bytes Diff had 6 unique methods, 3817 unique bytes Top file improvements by size (bytes): -1647 : System.Private.CoreLib.dasm (-0.05% of base) 1 total files with size differences (1 improved, 0 regressed), 0 unchanged. 

| Method | Toolchain | Key | Value | Size | Mean | Ratio | |------------ |---------- |--------|--------|----- |---------:|------:| | TryGetValue | base | Int32 | Int32 | 512 | 6.078 us | 1.00 | | TryGetValue | diff | Int32 | Int32 | 512 | 5.278 us | 0.87 | | TryGetValue | base | string | string | 512 | 16.24 us | 1.00 | | TryGetValue | diff | string | string | 512 | 15.50 us | 0.95 | 

/cc @jkotas @AntonLapounov @GrabYourPitchforks

[danmoseley]

Thanks @benaadams . Are you going to share microbenchmarks for this one?

[benaadams]

TryGetValue improved significantly

| Method | Toolchain | Mean | Ratio | |----------------------------------- |---------- |----------:|------:| | TryGetValue_17_Int_Int | base | 10.092 ns | 1.00 | | TryGetValue_17_Int_Int | diff | 8.389 ns | 0.83 | | TryGetValue_3k_Int_Int | base | 9.296 ns | 1.00 | | TryGetValue_3k_Int_Int | diff | 7.951 ns | 0.86 | | | | | | | TryGetValue_17_Int_32ByteValue | base | 10.674 ns | 1.00 | | TryGetValue_17_Int_32ByteValue | diff | 8.390 ns | 0.78 | | TryGetValue_3k_Int_32ByteValue | base | 9.761 ns | 1.00 | | TryGetValue_3k_Int_32ByteValue | diff | 8.107 ns | 0.83 | | | | | | | TryGetValue_17_Int_32ByteRefsValue | base | 13.908 ns | 1.00 | | TryGetValue_17_Int_32ByteRefsValue | diff | 8.314 ns | 0.60 | | TryGetValue_3k_Int_32ByteRefsValue | base | 13.550 ns | 1.00 | | TryGetValue_3k_Int_32ByteRefsValue | diff | 8.151 ns | 0.60 | 

ContainsKey more or less the same

| Method | Toolchain | Mean | Ratio | |----------------------------------- |---------- |----------:|------:| | ContainsValue_17_Int_Int | base | 7.702 ns | 1.00 | | ContainsValue_17_Int_Int | diff | 7.571 ns | 0.99 | | ContainsValue_3k_Int_Int | base | 7.828 ns | 1.00 | | ContainsValue_3k_Int_Int | diff | 7.388 ns | 0.94 | | | | | | | ContainsKey_17_Int_32ByteValue | base | 7.584 ns | 1.00 | | ContainsKey_17_Int_32ByteValue | diff | 7.707 ns | 1.01 | | ContainsKey_3k_Int_32ByteValue | base | 7.569 ns | 1.00 | | ContainsKey_3k_Int_32ByteValue | diff | 7.659 ns | 1.01 | | | | | | | ContainsKey_17_Int_32ByteRefsValue | base | 7.460 ns | 1.00 | | ContainsKey_17_Int_32ByteRefsValue | diff | 7.959 ns | 1.07 | | ContainsKey_3k_Int_32ByteRefsValue | base | 7.544 ns | 1.00 | | ContainsKey_3k_Int_32ByteRefsValue | diff | 7.940 ns | 1.05 | 

Not much movement on KNucleotide; guess that was mostly coming from dropping modulo

| Method | Toolchain | Mean | |-------------- |---------- |---------:| | KNucleotide_1 | base | 373.2 ms | | KNucleotide_1 | diff | 372.5 ms | | | | | | KNucleotide_9 | base | 94.92 ms | | KNucleotide_9 | diff | 92.40 ms | 

[benaadams]

A follow up; as @GrabYourPitchforks suggests #27149 (comment) would be to add the helper methods

partial class Unsafe { internal bool IsNullRef<T>(in T); internal ref T NullRef<T>(); }

Then drop the unsafe from the methods; however that requires Jit changes not to regress.

[benaadams]

Non-sequential keys are also improved

| Method | Toolchain | Result | Key | Value | Size | Mean | Ratio | |------------ |---------- |--------|--------|--------|----- |---------:|------:| | TryGetValue | base | true | Int32 | Int32 | 512 | 6.078 us | 1.00 | | TryGetValue | diff | true | Int32 | Int32 | 512 | 5.278 us | 0.87 | | TryGetValue | base | false | Int32 | Int32 | 512 | 8.839 us | 1.00 | | TryGetValue | diff | false | Int32 | Int32 | 512 | 5.629 us | 0.64 | | TryGetValue | base | true | string | string | 512 | 16.24 us | 1.00 | | TryGetValue | diff | true | string | string | 512 | 15.50 us | 0.95 | | TryGetValue | base | false | string | string | 512 | 14.22 us | 1.00 | | TryGetValue | diff | false | string | string | 512 | 12.82 us | 0.90 | 

[benaadams]

Linux_musl still queued and ci timed out because of it :(

[jkotas]

Then drop the unsafe from the methods; however that requires Jit changes not to regress.

I would like to see this done as part of this change. Are you sure that it actually needs jit changes? I would expect that the two new Unsafe methods just needs implemented in the IL in getILIntrinsicImplementationForUnsafe.

[jkotas]

Unsafe.NullRef, Unsafe.IsNullRef

Here is a bit of code that you reuse: 9353ac3#diff-bbced44286fd2ad6bcf70fbd3c276d57L7254

Also, I have submitted a proposal to add these two APIs to a public version of Unsafe as well: https://github.com/dotnet/corefx/issues/41783

[benaadams]

Are you sure that it actually needs jit changes?

Found out where the weird boolean asm output is coming from; is basically the Jit doing as its asked

ldarg.0 ldc.i4.0 conv.u ceq ldc.i4.0 ceq ret 

Can maybe do something like this instead?

ldarg.0 ldc.i4.0 conv.u cgt.un ret 

/cc @AndyAyersMS

[EgorBo]

Found out where the weird boolean asm output is coming from; is basically the Jit doing as its asked

I think my PR #27167 should improve codegen for this IL if I understand you correctly.

[benaadams]

I think my PR #27167 should improve codegen for this IL if I understand you correctly.

Might do :) Junky bool handling

[benaadams]

I'm having issue with the metasig.h for ref T NullRef<T>()

I've got to

DEFINE_METASIG(GM(RetRefT, IMAGE_CEE_CS_CALLCONV_DEFAULT, 1, _, r(M(0)))) 

but its not very happy with that; any pointers?

[GrabYourPitchforks]

You shouldn't need to define a metasig since the method doesn't have multiple overloads. Check 9353ac3#diff-027a321a370548745efff3d113085fd6L716, which specifies NoSig to mean "don't bother with signature matching - only match on the name."

[benaadams]

which specifies NoSig to mean "don't bother with signature matching - only match on the name."

Ah ah! I was trying to work out the rules on how some things had nosig when clearly they did; and how that fit with the other sig rules :)

[benaadams]

Final scores:

Total bytes of diff: -1647 (-0.05% of base) diff is an improvement. Total byte diff includes -118 bytes from reconciling methods Base had 4 unique methods, 3935 unique bytes Diff had 6 unique methods, 3817 unique bytes Top file improvements by size (bytes): -1647 : System.Private.CoreLib.dasm (-0.05% of base) 1 total files with size differences (1 improved, 0 regressed), 0 unchanged. Top method regressions by size (bytes): 2075 ( 8 of base) : System.Private.CoreLib.dasm - Dictionary`2:FindValue(ref):byref:this (0 base, 5 diff methods) 1029 ( 8 of base) : System.Private.CoreLib.dasm - Dictionary`2:FindValue(long):byref:this (0 base, 3 diff methods) 343 ( 8 of base) : System.Private.CoreLib.dasm - Dictionary`2:FindValue(struct):byref:this (0 base, 1 diff methods) 327 ( 8 of base) : System.Private.CoreLib.dasm - Dictionary`2:FindValue(int):byref:this (0 base, 1 diff methods) 33 ( 8 of base) : System.Private.CoreLib.dasm - Unsafe:NullRef():byref (0 base, 11 diff methods) 23 ( 2.44% of base) : System.Private.CoreLib.dasm - ResourceManager:InternalGetResourceSet(ref,bool,bool):ref:this 17 ( 8.72% of base) : System.Private.CoreLib.dasm - EventRegistrationTokenTable`1:AddEventHandlerNoLock(ref):struct:this 10 ( 8 of base) : System.Private.CoreLib.dasm - Unsafe:IsNullRef(byref):bool (0 base, 1 diff methods) 5 ( 4.35% of base) : System.Private.CoreLib.dasm - Dictionary`2:ContainsKey(ref):bool:this (5 methods) 4 ( 0.34% of base) : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetCustomAttributes(ref,ref,byref) (4 methods) 4 ( 0.44% of base) : System.Private.CoreLib.dasm - PseudoCustomAttribute:IsDefined(ref,ref):bool (4 methods) 4 ( 0.78% of base) : System.Private.CoreLib.dasm - Dictionary`2:ContainsValue(struct):bool:this (3 methods) 3 ( 0.04% of base) : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (84 methods) 3 ( 4.35% of base) : System.Private.CoreLib.dasm - Dictionary`2:ContainsKey(long):bool:this (3 methods) 2 ( 0.08% of base) : System.Private.CoreLib.dasm - KeyCollection:CopyTo(ref,int):this (13 methods) 2 ( 0.08% of base) : System.Private.CoreLib.dasm - ValueCollection:CopyTo(ref,int):this (13 methods) 2 ( 0.04% of base) : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this (12 methods) 2 ( 0.04% of base) : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this (12 methods) 2 ( 4.35% of base) : System.Private.CoreLib.dasm - Dictionary`2:ContainsKey(struct):bool:this (2 methods) 1 ( 0.60% of base) : System.Private.CoreLib.dasm - Attribute:CopyToArrayList(ref,ref,ref) 1 ( 0.06% of base) : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateInterfaces(struct):ref:this 1 ( 0.14% of base) : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateEvents(struct,ref,ref,byref):this 1 ( 0.10% of base) : System.Private.CoreLib.dasm - ManyElementAsyncLocalValueMap:Set(ref,ref,bool):ref:this 1 ( 0.49% of base) : System.Private.CoreLib.dasm - SerializationInfo:AddValueInternal(ref,ref,ref):this 1 ( 0.56% of base) : System.Private.CoreLib.dasm - EventPipeEventDispatcher:RemoveEventListener(ref):this Top method improvements by size (bytes): -2270 (-100.00% of base) : System.Private.CoreLib.dasm - Dictionary`2:FindEntry(ref):int:this (5 base, 0 diff methods) -1008 (-100.00% of base) : System.Private.CoreLib.dasm - Dictionary`2:FindEntry(long):int:this (3 base, 0 diff methods) -336 (-100.00% of base) : System.Private.CoreLib.dasm - Dictionary`2:FindEntry(struct):int:this (1 base, 0 diff methods) -321 (-100.00% of base) : System.Private.CoreLib.dasm - Dictionary`2:FindEntry(int):int:this (1 base, 0 diff methods) -308 (-20.40% of base) : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Contains(struct):bool:this (11 methods) -274 (-17.86% of base) : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.IDictionary.get_Item(ref):ref:this (11 methods) -253 (-14.82% of base) : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Remove(struct):bool:this (11 methods) -154 (-37.11% of base) : System.Private.CoreLib.dasm - Dictionary`2:TryGetValue(ref,byref):bool:this (5 methods) -96 (-37.65% of base) : System.Private.CoreLib.dasm - Dictionary`2:TryGetValue(long,byref):bool:this (3 methods) -93 (-43.06% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(long):ref:this (3 methods) -61 (-34.46% of base) : System.Private.CoreLib.dasm - Dictionary`2:TryGetValue(struct,byref):bool:this (2 methods) -46 (-20.18% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(ref):struct:this (2 methods) -36 (-21.95% of base) : System.Private.CoreLib.dasm - Unsafe:AsRef(long):byref (41 base, 32 diff methods) -32 (-37.65% of base) : System.Private.CoreLib.dasm - Dictionary`2:TryGetValue(int,byref):bool:this -31 (-44.93% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(int):ref:this -31 (-43.06% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(struct):ref:this -29 (-21.80% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(struct):struct:this -25 (-2.16% of base) : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.IDictionary.Contains(ref):bool:this (11 methods) -23 (-25.84% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(ref):ref:this -23 (-25.84% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(ref):bool:this -23 (-26.14% of base) : System.Private.CoreLib.dasm - Dictionary`2:get_Item(ref):int:this -15 (-17.24% of base) : System.Private.CoreLib.dasm - KeyCollection:System.Collections.Generic.ICollection<TKey>.Contains(long):bool:this (3 methods) -12 (-2.55% of base) : System.Private.CoreLib.dasm - ManifestBuilder:GetKeywords(long,ref):ref:this -11 (-3.91% of base) : System.Private.CoreLib.dasm - Attribute:AddAttributesToList(ref,ref,ref) -10 (-0.53% of base) : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateProperties(struct,ref,ref,ref,byref):this 

[benaadams]

Plus verify that nothing went awry in the later commits

| Method | Toolchain | Mean | Ratio | |----------------------------------- |---------- |----------:|------:| | TryGetValue_17_Int_Int | base | 9.944 ns | 1.00 | | TryGetValue_17_Int_Int | diff | 8.425 ns | 0.85 | | TryGetValue_3k_Int_Int | base | 9.399 ns | 1.00 | | TryGetValue_3k_Int_Int | diff | 8.020 ns | 0.85 | | | | | | | TryGetValue_17_Int_32ByteValue | base | 10.421 ns | 1.00 | | TryGetValue_17_Int_32ByteValue | diff | 8.435 ns | 0.81 | | TryGetValue_3k_Int_32ByteValue | base | 9.934 ns | 1.00 | | TryGetValue_3k_Int_32ByteValue | diff | 8.022 ns | 0.81 | | | | | | | TryGetValue_17_Int_32ByteRefsValue | base | 14.197 ns | 1.00 | | TryGetValue_17_Int_32ByteRefsValue | diff | 8.382 ns | 0.59 | | TryGetValue_3k_Int_32ByteRefsValue | base | 13.710 ns | 1.00 | | TryGetValue_3k_Int_32ByteRefsValue | diff | 8.010 ns | 0.58 | 

/cc @mjsabby

[jkotas]

Thank you!

[jkotas]

Could you please add the IL for the NullRef and IsNullRef also to https://github.com/dotnet/coreclr/blob/master/src/tools/crossgen2/Common/TypeSystem/IL/Stubs/UnsafeIntrinsics.cs#L72 ?

[benaadams]

Could you please add the IL for the NullRef and IsNullRef also to

Added