Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RyuJIT] Devirtualize Comparer<T>.Default #48160

Merged
merged 9 commits into from
Feb 12, 2021

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 11, 2021

See #43796

Mostly a copy-paste from the existing EqualityComparer<T>.Default devirtualization.
Example:

private static int Compare(byte a, byte b) => Comparer<byte>.Default.Compare(a, b);

master:

; Method Program:Compare(ubyte,ubyte):int
G_M13367_IG01:              ;; offset=0000H
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       8BF1                 mov      esi, ecx
       8BFA                 mov      edi, edx
G_M13367_IG02:              ;; offset=000AH
       48B928002535FF7F0000 mov      rcx, 0x7FFF35250028
       BA08000000           mov      edx, 8
       E8F218755F           call     CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS
       48B9D02C0098F8010000 mov      rcx, 0x1F898002CD0
       488B09               mov      rcx, gword ptr [rcx]
       400FB6D6             movzx    rdx, sil
       440FB6C7             movzx    r8, dil
       488B01               mov      rax, qword ptr [rcx]
       488B4048             mov      rax, qword ptr [rax+72]
       488B4020             mov      rax, qword ptr [rax+32]
G_M13367_IG03:              ;; offset=003EH
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       48FFE0               rex.jmp  rax
; Total bytes of code: 71

PR:

; Method Program:Compare(ubyte,ubyte):int
G_M13367_IG01:              ;; offset=0000H
G_M13367_IG02:              ;; offset=0000H
       0FB6C1               movzx    rax, cl
       0FB6D2               movzx    rdx, dl
       2BC2                 sub      eax, edx
G_M13367_IG03:              ;; offset=0008H
       C3                   ret      
; Total bytes of code: 9

JIT diff is empty but it should allow to write generic versions of Max<T>/Min<T> functions (#43796).

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 11, 2021
@stephentoub
Copy link
Member

Sweet!

@stephentoub
Copy link
Member

cc: @eiriktsarpalis

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Seems like the enum comparer gets inlined and optimized w/o needing annotation, but can you double-check?

Did you try crossgenning your test and then running it? Would be good to get somebody more familiar with crossgen2 to sign off on the changes there.

Changes LGTM, just some comments on comments.

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/flowgraph.cpp Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Feb 11, 2021

Seems like the enum comparer gets inlined and optimized w/o needing annotation, but can you double-check?

Did you try crossgenning your test and then running it? Would be good to get somebody more familiar with crossgen2 to sign off on the changes there.

Changes LGTM, just some comments on comments.

Yeah, the tests pass with crossgen2

@AndyAyersMS
Copy link
Member

@dotnet/crossgen-contrib can somebody take a quick look at the crossgen2 changes here?

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Crossgen2-related changes look good to me, thank you!

@eiriktsarpalis
Copy link
Member

I ran my PriorityQueue benchmarks against the PR branch: this compares a heap implementation using Comparer<T>.Default.Compare calls versus an identical one using IComparable:

master branch

Method Size Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
PriorityQueue_ComparerDefault 10 502.7 ns 9.83 ns 13.13 ns - - - -
PriorityQueue_Comparable 10 353.8 ns 6.86 ns 7.90 ns - - - -
PriorityQueue_ComparerDefault 50 4,171.1 ns 76.74 ns 68.02 ns - - - -
PriorityQueue_Comparable 50 2,380.8 ns 37.08 ns 32.87 ns - - - -
PriorityQueue_ComparerDefault 150 20,487.9 ns 364.63 ns 341.08 ns - - - -
PriorityQueue_Comparable 150 11,740.0 ns 138.82 ns 123.06 ns - - - -
PriorityQueue_ComparerDefault 500 92,828.3 ns 1,767.72 ns 1,567.04 ns - - - -
PriorityQueue_Comparable 500 64,291.4 ns 1,196.99 ns 1,378.46 ns - - - -
PriorityQueue_ComparerDefault 1000 205,028.1 ns 3,697.23 ns 4,257.74 ns - - - -
PriorityQueue_Comparable 1000 147,682.6 ns 2,176.21 ns 1,929.15 ns - - - -
PriorityQueue_ComparerDefault 10000 2,718,728.5 ns 54,288.34 ns 58,087.92 ns - - - 1 B
PriorityQueue_Comparable 10000 1,935,928.9 ns 36,901.95 ns 34,518.11 ns - - - 1 B
PriorityQueue_ComparerDefault 1000000 454,903,575.0 ns 8,828,957.20 ns 11,480,138.23 ns - - - 144 B
PriorityQueue_Comparable 1000000 348,606,720.0 ns 6,908,734.31 ns 6,462,434.36 ns - - - 2360 B

PR branch

Method Size Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
PriorityQueue_ComparerDefault 10 349.2 ns 6.19 ns 7.83 ns - - - -
PriorityQueue_Comparable 10 327.2 ns 6.28 ns 5.87 ns - - - -
PriorityQueue_ComparerDefault 50 2,514.8 ns 18.71 ns 14.61 ns - - - -
PriorityQueue_Comparable 50 2,274.4 ns 37.11 ns 32.89 ns - - - -
PriorityQueue_ComparerDefault 150 12,616.7 ns 54.33 ns 45.37 ns - - - -
PriorityQueue_Comparable 150 11,299.3 ns 96.31 ns 90.09 ns - - - -
PriorityQueue_ComparerDefault 500 66,912.2 ns 541.71 ns 506.72 ns - - - -
PriorityQueue_Comparable 500 63,575.9 ns 375.87 ns 333.20 ns - - - -
PriorityQueue_ComparerDefault 1000 152,907.1 ns 554.02 ns 518.23 ns - - - -
PriorityQueue_Comparable 1000 147,177.2 ns 1,111.73 ns 1,039.92 ns - - - -
PriorityQueue_ComparerDefault 10000 2,060,862.3 ns 33,190.78 ns 29,422.76 ns - - - 1 B
PriorityQueue_Comparable 10000 1,965,759.2 ns 39,021.23 ns 53,412.70 ns - - - 1 B
PriorityQueue_ComparerDefault 1000000 372,963,264.3 ns 7,253,139.17 ns 6,429,718.95 ns - - - 760 B
PriorityQueue_Comparable 1000000 362,177,334.8 ns 7,176,855.04 ns 9,076,423.69 ns - - - 144 B

@AndyAyersMS
Copy link
Member

Nice work @EgorBo!

I ran my PriorityQueue benchmarks ...

@adamsitnik Would be nice to fix the benchmark harness so you can pass multiple --corerun args and have it compare them, like it can do for --runtimes.

@stephentoub
Copy link
Member

Would be nice to fix the benchmark harness so you can pass multiple --corerun args and have it compare them, like it can do for --runtimes.

You can, unless I'm misunderstanding what you're asking for. I do this all the time:

--corerun d:\coreclrtest\master\corerun.exe d:\coreclrtest\pr\corerun.exe

@AndyAyersMS
Copy link
Member

I do this all the time ...

Guess I'm confused. Maybe what I was thinking was allowing a mixture of --runtimes and --corerun .

@akoeplinger
Copy link
Member

@EgorBo this seems to cause a warning in CI, would you mind fixing it:

/__w/1/s/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs(240,32): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [/__w/1/s/src/tests/JIT/opt/Devirtualization/Comparer_get_Default.csproj]

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants