Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Improve Dictionary TryGetValue size/perfomance #27195

Merged
merged 8 commits into from
Oct 16, 2019
Merged

Improve Dictionary TryGetValue size/perfomance #27195

merged 8 commits into from
Oct 16, 2019

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Oct 15, 2019 •
edited
Loading

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

@benaadams benaadams mentioned this pull request Oct 15, 2019
Closed
@danmoseley
Copy link
Member

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

@benaadams
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@jkotas
Copy link
Member

jkotas commented Oct 15, 2019

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
Copy link
Member

jkotas commented Oct 15, 2019

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
Copy link
Member Author

benaadams commented Oct 15, 2019 •
edited
Loading

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
Copy link
Member

EgorBo commented Oct 15, 2019

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
Copy link
Member Author

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

Might do :) Junky bool handling

@benaadams
Copy link
Member Author

benaadams commented Oct 15, 2019 •
edited
Loading

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
Copy link
Member

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
Copy link
Member Author

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 :)

jkotas
jkotas reviewed Oct 15, 2019
View reviewed changes
src/vm/jitinterface.cpp Outdated Show resolved Hide resolved
@benaadams
Copy link
Member Author

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
Copy link
Member Author

benaadams commented Oct 16, 2019 •
edited
Loading

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

@benaadams @jkotas
Apply suggestions from code review
bbf8fc7 
Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
jkotas
jkotas approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jkotas
Copy link
Member

jkotas commented Oct 16, 2019

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
Copy link
Member Author

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

Added

@jkotas jkotas merged commit 921b39c into dotnet:master Oct 16, 2019
@benaadams benaadams deleted the dict-entry-no-mod branch October 16, 2019 15:28
@benaadams benaadams mentioned this pull request Oct 19, 2019
Merged
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Oct 21, 2019
@benaadams @dotnet-bot
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
e77acb8 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Oct 21, 2019
@benaadams @dotnet-bot
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
ce528c9 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Oct 21, 2019
@benaadams @dotnet-bot
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
824390d 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
safern pushed a commit to dotnet/corefx that referenced this pull request Oct 21, 2019
@benaadams @safern
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
e8c4f71 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Oct 22, 2019
@benaadams @jkotas
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
4a0d843 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar pushed a commit to mono/mono that referenced this pull request Oct 22, 2019
@benaadams @marek-safar
Improve Dictionary TryGetValue size/perfomance (dotnet/coreclr#27195)
d5b182c 
* Dictionary avoid second bounds check in Get methods

* Add NullRef methods to Unsafe

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@jkotas jkotas mentioned this pull request Feb 1, 2020
Closed
@AndyAyersMS AndyAyersMS mentioned this pull request Aug 14, 2020
Closed
@MihaZupan MihaZupan mentioned this pull request Feb 11, 2021
Closed
@jkotas jkotas mentioned this pull request Apr 23, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EgorBo EgorBo EgorBo left review comments

@AntonLapounov AntonLapounov AntonLapounov left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

@jkotas jkotas jkotas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@benaadams @danmoseley @jkotas @EgorBo @GrabYourPitchforks @AntonLapounov