Non-generic StringComparer conversion for dict and hashset #49608

benaadams · 2021-03-14T18:07:28Z

Move the string comparer switching out of the generic dictionary (it only applies to string) so doesn't need to be there for every shared generic.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3196205
Total bytes of diff: 3195323
Total bytes of delta: -882 (-0.03% of base)
    diff is an improvement.


Total byte diff includes 108 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    1 unique methods,      108 unique bytes

Top file improvements (bytes):
        -882 : System.Private.CoreLib.dasm (-0.03% of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method regressions (bytes):
         108 (     ∞ of base) : System.Private.CoreLib.dasm - NonRandomizedStringEqualityComparer:GetStringComparer(Object):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (bytes):
        -825 (-40.05% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)
        -165 (-54.82% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this

Top method regressions (percentages):
         108 (     ∞ of base) : System.Private.CoreLib.dasm - NonRandomizedStringEqualityComparer:GetStringComparer(Object):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (percentages):
        -165 (-54.82% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this
        -825 (-40.05% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)

3 total methods with Code Size differences (2 improved, 1 regressed), 21241 unchanged.

ghost · 2021-03-14T18:07:36Z

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

Issue Details

Move the string comparer switching out of the generic dictionary (it only applies to string) so doesn't need to be there for every shared generic.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3196205
Total bytes of diff: 3195116
Total bytes of delta: -1089 (-0.03% of base)
    diff is an improvement.


Total byte diff includes 95 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    1 unique methods,       95 unique bytes

Top file improvements (bytes):
       -1089 : System.Private.CoreLib.dasm (-0.03% of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method regressions (bytes):
          95 (     ∞ of base) : System.Private.CoreLib.dasm - HashHelpers:GetStringComparer(IEqualityComparer`1):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (bytes):
        -990 (-48.06% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)
        -194 (-64.45% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this

Top method regressions (percentages):
          95 (     ∞ of base) : System.Private.CoreLib.dasm - HashHelpers:GetStringComparer(IEqualityComparer`1):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (percentages):
        -194 (-64.45% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this
        -990 (-48.06% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)

3 total methods with Code Size differences (2 improved, 1 regressed), 21241 unchanged.

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

benaadams · 2021-03-14T18:15:14Z

G_M17833_IG01:
       push     rdi
       push     rsi
-      push     rbp
-      push     rbx
       sub      rsp, 40
       mov      qword ptr [rsp+20H], rcx
       mov      rsi, rcx
       mov      rdi, r8

...

 G_M17833_IG07:
-       mov      rbx, qword ptr [rsi]
-       mov      rcx, rbx
+       mov      rcx, qword ptr [rsi]
        call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
        cmp      rax, qword ptr [(reloc)]
-       jne      G_M17833_IG13
+       jne      SHORT G_M17833_IG09

 G_M17833_IG08:
-       mov      rdi, gword ptr [rsi+24]
-       test     rdi, rdi
-       jne      SHORT G_M17833_IG10
-       mov      rcx, rbx
-       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1838H]
-       mov      rcx, rdi
-       call     [CORINFO_HELP_CHKCASTANY]
+       mov      rcx, gword ptr [rsi+24]
+       call     [NonRandomizedStringEqualityComparer:GetStringComparer(IEqualityComparer`1):IEqualityComparer`1]
        lea      rcx, bword ptr [rsi+24]
        mov      rdx, rax
        call     [CORINFO_HELP_ASSIGN_REF]
-       nop      

 G_M17833_IG09:
-       add      rsp, 40
-       pop      rbx
-       pop      rbp
-       pop      rsi
-       pop      rdi
-       ret      

-G_M17833_IG10:
-       mov      rbp, rdi
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       cmp      gword ptr [rax+856], rbp
-       jne      SHORT G_M17833_IG12
-       mov      rcx, rbx
-       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1840H]
-       mov      rcx, rdi
-       call     [CORINFO_HELP_CHKCASTANY]
-       lea      rcx, bword ptr [rsi+24]
-       mov      rdx, rax
-       call     [CORINFO_HELP_ASSIGN_REF]
        nop      

-G_M17833_IG11:
-       add      rsp, 40
-       pop      rbx
-       pop      rbp
-       pop      rsi
-       pop      rdi
-       ret      

-G_M17833_IG12:
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       cmp      gword ptr [rax+864], rdi
-       jne      SHORT G_M17833_IG13
-       mov      rcx, rbx
-       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1848H]
-       mov      rcx, rdi
-       call     [CORINFO_HELP_CHKCASTANY]
-       lea      rcx, bword ptr [rsi+24]
-       mov      rdx, rax
-       call     [CORINFO_HELP_ASSIGN_REF]

-G_M17833_IG13:
-       nop      

-G_M17833_IG14:
+G_M17833_IG10:
        add      rsp, 40
-       pop      rbx
-       pop      rbp
        pop      rsi
        pop      rdi
        ret

GrabYourPitchforks · 2021-03-14T19:56:34Z

95 ( ∞ of base)

Lulz. :)

GrabYourPitchforks

LGTM. @jkotas - thoughts on the call to Unsafe.As<IEqualityComparer<string>>(object)? I would think this generic instantiation would already exist in every real-world .NET application, but are there situations where we need to avoid it? And if so, would it make sense to make the new GetStringComparer helper method take IEqualityComparer instead of IEqualityComparer<string>?

I wouldn't want to contort the code if this is unnecessary.

benaadams · 2021-03-14T21:29:34Z

The non Unsafe version

_comparer = (IEqualityComparer<T>?)NonRandomizedStringEqualityComparer
    .GetStringComparer((IEqualityComparer<string>?)_comparer);

vs the Unsafe

_comparer = Unsafe.As<IEqualityComparer<TKey>?>(
    NonRandomizedStringEqualityComparer.GetStringComparer(
        Unsafe.As<IEqualityComparer<string>?>(_comparer)));

The regular casting includes a double cast even though its passed the typeof(TKey) == typeof(string) guard which implies the casts will always succeed.

G_M17833_IG07:
       mov      rcx, qword ptr [rsi]
       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
       cmp      rax, qword ptr [(reloc)]
       jne      SHORT G_M17833_IG09
						;; bbWeight=1    PerfScore 8.00
G_M17833_IG08:
-      mov      rcx, qword ptr [rsi]
-      call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-      mov      rdi, rax
       mov      rcx, gword ptr [rsi+24]
-      call     [CORINFO_HELP_READYTORUN_CHKCAST]
-      mov      rcx, rax
       call     [NonRandomizedStringEqualityComparer:GetStringComparer(IEqualityComparer`1):IEqualityComparer`1]
-      mov      rdx, rax
-      mov      rcx, rdi
-      call     [CORINFO_HELP_CHKCASTANY]
       lea      rcx, bword ptr [rsi+24]
       mov      rdx, rax
       call     [CORINFO_HELP_ASSIGN_REF]
-					;; bbWeight=0.50 PerfScore 10.38
+					;; bbWeight=0.50 PerfScore 4.38
G_M17833_IG09:
       nop      
						;; bbWeight=1    PerfScore 0.25

jkotas · 2021-03-14T21:55:29Z

comparer switching out of the generic dictionary (it only applies to string) so doesn't need to be there for every shared generic.

Is it there for every shared generic? The JIT should be able to see that the if (typeof(TKey) == typeof(string)) can be only true when TKey == Canon and dead code eliminate it for all other cases.

So far we have been able to avoid introducing type-unsafe code in collection types. This would be the first instance of type-unsafe code in Dictionary that I think is very unfortunate. I think we should rather look at fixing the JIT to get the code optimized in similar way instead.

jkotas

I would like to see what it would take to fix the JIT instead.

benaadams · 2021-03-14T21:56:50Z

Was wondering why new Dictionary<string, object?>(count) was calling CastHelpers.ChkCastAny

benaadams · 2021-03-14T22:20:01Z

I would like to see what it would take to fix the JIT instead.

Raised issue #49614

benaadams · 2021-03-14T22:24:59Z

Is it there for every shared generic?

Looks to be; it is however eliminated for struct types as they can never be string

jkotas · 2021-03-14T22:38:41Z

So I guess that part of the problem is that the check is not eliminated for Dictionary<MyGenericStruct<object>, int>. Is that correct?

benaadams · 2021-03-14T22:45:00Z

Looks to be eliminated for Dictionary<MyGenericStruct<object>, int> but not for Dictionary<object, int> or Dictionary<string, int> etc

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

...System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs

jkotas · 2021-03-14T22:49:38Z

Looks to be eliminated for Dictionary<MyGenericStruct<object>, int> but not for Dictionary<object, int> or Dictionary<string, int> et

Ok, so this part works as expected. It cannot be eliminated for Dictionary<object, int> and Dictionary<string, int> since they share the same code.

benaadams · 2021-03-14T23:23:21Z

So is only one cast in the .ctor which could in theory be eliminated by the Jit as its behind a type guard confirming TKey is string so IEqualityComparer<string> should be always castable to IEqualityComparer<TKey> #49614

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3196205
Total bytes of diff: 3195323
Total bytes of delta: -882 (-0.03% of base)
    diff is an improvement.


Total byte diff includes 108 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    1 unique methods,      108 unique bytes

Top file improvements (bytes):
        -882 : System.Private.CoreLib.dasm (-0.03% of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method regressions (bytes):
         108 (     ∞ of base) : System.Private.CoreLib.dasm - NonRandomizedStringEqualityComparer:GetStringComparer(Object):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (bytes):
        -825 (-40.05% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)
        -165 (-54.82% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this

Top method regressions (percentages):
         108 (     ∞ of base) : System.Private.CoreLib.dasm - NonRandomizedStringEqualityComparer:GetStringComparer(Object):IEqualityComparer`1 (0 base, 1 diff methods)

Top method improvements (percentages):
        -165 (-54.82% of base) : System.Private.CoreLib.dasm - HashSet`1:.ctor(IEqualityComparer`1):this
        -825 (-40.05% of base) : System.Private.CoreLib.dasm - Dictionary`2:.ctor(int,IEqualityComparer`1):this (10 methods)

3 total methods with Code Size differences (2 improved, 1 regressed), 21241 unchanged.

benaadams · 2021-03-14T23:50:27Z

G_M17833_IG01:
       push     rdi
       push     rsi
-      push     rbp
-      push     rbx
       sub      rsp, 40
       mov      qword ptr [rsp+20H], rcx
       mov      rsi, rcx
       mov      rdi, r8

...

 G_M17833_IG07:
-       mov      rbx, qword ptr [rsi]
-       mov      rcx, rbx
+       mov      rcx, qword ptr [rsi]
        call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
        cmp      rax, qword ptr [(reloc)]
-       jne      G_M17833_IG13
+       jne      SHORT G_M17833_IG09

 G_M17833_IG08:
-       mov      rdi, gword ptr [rsi+24]
-       test     rdi, rdi
-       jne      SHORT G_M17833_IG10
-       mov      rcx, rbx
-       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1838H]
-       mov      rcx, rdi
-       call     [CORINFO_HELP_CHKCASTANY]
+       mov      rcx, gword ptr [rsi+24]
+       call     [NonRandomizedStringEqualityComparer:GetStringComparer(Object):IEqualityComparer`1]
+       mov      rdi, rax
+       test     rdi, rdi
+       je       SHORT G_M17833_IG09
+       mov      rcx, qword ptr [rsi]
-       lea      rcx, bword ptr [rsi+24]
-       mov      rdx, rax
-       call     [CORINFO_HELP_ASSIGN_REF]
-       nop      

-G_M17833_IG09:
-       add      rsp, 40
-       pop      rbx
-       pop      rbp
-       pop      rsi
-       pop      rdi
-       ret      

-G_M17833_IG10:
-       mov      rbp, rdi
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       cmp      gword ptr [rax+856], rbp
-       jne      SHORT G_M17833_IG12
-       mov      rcx, rbx
-       call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1840H]
-       mov      rcx, rdi
-       call     [CORINFO_HELP_CHKCASTANY]
-       lea      rcx, bword ptr [rsi+24]
-       mov      rdx, rax
-       call     [CORINFO_HELP_ASSIGN_REF]
        nop      

-G_M17833_IG11:
-       add      rsp, 40
-       pop      rbx
-       pop      rbp
-       pop      rsi
-       pop      rdi
-       ret      

-G_M17833_IG12:
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       cmp      gword ptr [rax+864], rdi
-       jne      SHORT G_M17833_IG13
-       mov      rcx, rbx
        call     [CORINFO_HELP_READYTORUN_GENERIC_HANDLE]
+       mov      rcx, rax
-       mov      rdi, rax
-       call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
-       mov      rdx, gword ptr [rax+1848H]
        mov      rcx, rdi
        call     [CORINFO_HELP_CHKCASTANY]
        lea      rcx, bword ptr [rsi+24]
        mov      rdx, rax
        call     [CORINFO_HELP_ASSIGN_REF]

-G_M17833_IG13:
+G_M17833_IG09:
        nop      

-G_M17833_IG14:
+G_M17833_IG10:
        add      rsp, 40
-       pop      rbx
-       pop      rbp
        pop      rsi
        pop      rdi
        ret

...

-; Total bytes of code 334, prolog size 13, PerfScore 95.78, instruction count 95, allocated bytes for code 334 (MethodHash=4fbeba56) for method Dictionary`2:.ctor(int,IEqualityComparer`1):this
+; Total bytes of code 169, prolog size 11, PerfScore 52.53, instruction count 48, allocated bytes for code 169 (MethodHash=4fbeba56) for method Dictionary`2:.ctor(int,IEqualityComparer`1):this

jkotas

LGTM. Thank you!

I think this may make constructing Dictionary<string, ...> a tiny bit slower. I think it is fine price to pay to eliminate the duplication.

dotnet-issue-labeler bot added the area-System.Collections label Mar 14, 2021

Non-generic casting

227be0b

benaadams force-pushed the dict branch from fbae294 to 227be0b Compare March 14, 2021 18:36

GrabYourPitchforks approved these changes Mar 14, 2021

View reviewed changes

Casts are to nullable type

fca3a24

jkotas requested changes Mar 14, 2021

View reviewed changes

benaadams mentioned this pull request Mar 14, 2021

Type check in shared generic doesn't eliminate casts to/from confirmed exact type #49614

Open

Use safe casting

4247f57

jkotas reviewed Mar 14, 2021

View reviewed changes

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

jkotas reviewed Mar 14, 2021

View reviewed changes

...System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs Outdated Show resolved Hide resolved

Feedback

0708143

Reduced casts by one

cd1d101

benaadams mentioned this pull request Mar 15, 2021

System.Threading.Thread tests crashing on Mono #49583

Closed

jkotas approved these changes Mar 15, 2021

View reviewed changes

jkotas merged commit 5ef6a06 into dotnet:main Mar 15, 2021

benaadams deleted the dict branch March 15, 2021 03:46

runfoapp bot mentioned this pull request Mar 15, 2021

Mono System.Text.Json.Tests on Windows timing out #42677

Open

ghost locked as resolved and limited conversation to collaborators Apr 14, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-generic StringComparer conversion for dict and hashset #49608

Non-generic StringComparer conversion for dict and hashset #49608

benaadams commented Mar 14, 2021 •

edited

Loading

ghost commented Mar 14, 2021

benaadams commented Mar 14, 2021 •

edited

Loading

GrabYourPitchforks commented Mar 14, 2021

GrabYourPitchforks left a comment

benaadams commented Mar 14, 2021 •

edited

Loading

jkotas commented Mar 14, 2021

jkotas left a comment

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021

jkotas commented Mar 14, 2021

benaadams commented Mar 14, 2021

jkotas commented Mar 14, 2021 •

edited

Loading

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021 •

edited

Loading

jkotas left a comment

Non-generic StringComparer conversion for dict and hashset #49608

Non-generic StringComparer conversion for dict and hashset #49608

Conversation

benaadams commented Mar 14, 2021 • edited Loading

ghost commented Mar 14, 2021

benaadams commented Mar 14, 2021 • edited Loading

GrabYourPitchforks commented Mar 14, 2021

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

benaadams commented Mar 14, 2021 • edited Loading

jkotas commented Mar 14, 2021

jkotas left a comment

Choose a reason for hiding this comment

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021

jkotas commented Mar 14, 2021

benaadams commented Mar 14, 2021

jkotas commented Mar 14, 2021 • edited Loading

benaadams commented Mar 14, 2021

benaadams commented Mar 14, 2021 • edited Loading

jkotas left a comment

Choose a reason for hiding this comment

benaadams commented Mar 14, 2021 •

edited

Loading

benaadams commented Mar 14, 2021 •

edited

Loading

benaadams commented Mar 14, 2021 •

edited

Loading

jkotas commented Mar 14, 2021 •

edited

Loading

benaadams commented Mar 14, 2021 •

edited

Loading