[RyuJIT] Devirtualize Comparer<T>.Default by EgorBo · Pull Request #48160 · dotnet/runtime

EgorBo · 2021-02-11T13:02:06Z

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

stephentoub · 2021-02-11T14:29:47Z

Sweet!

stephentoub · 2021-02-11T14:31:53Z

cc: @eiriktsarpalis

AndyAyersMS

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

src/coreclr/jit/flowgraph.cpp

EgorBo · 2021-02-11T18:53:07Z

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 · 2021-02-11T20:03:19Z

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

trylek

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

eiriktsarpalis · 2021-02-11T21:19:27Z

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 · 2021-02-12T17:26:38Z

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 · 2021-02-12T17:47:54Z

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 · 2021-02-12T19:10:22Z

I do this all the time ...

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

akoeplinger · 2021-02-23T12:31:38Z

@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]

Devirtualize Comparer<T>.Default

afa7a60

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 11, 2021

Update jitinterface.cpp

5d7711f

EgorBo added 2 commits February 11, 2021 18:05

Add more test cases

8b16218

Add Nullable<> support

f49b89e

AndyAyersMS approved these changes Feb 11, 2021

View reviewed changes

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

src/coreclr/jit/flowgraph.cpp Show resolved Hide resolved

EgorBo added 2 commits February 11, 2021 21:37

Address feedback

b2303e6

Add more test cases

5c0b3d5

EgorBo force-pushed the jit-devirt-comparer-default branch from 80a75a1 to 5c0b3d5 Compare February 11, 2021 18:49

Address feedback

18821b4

trylek approved these changes Feb 11, 2021

View reviewed changes

EgorBo added 2 commits February 12, 2021 01:57

Disable the test on Mono

67b1509

Update issues.targets

7a11cf7

runfoapp bot mentioned this pull request Feb 12, 2021

System.Threading.Tasks.Tests timed out on net5.0-Linux-Debug-arm64-Mono_release #42024

Closed

EgorBo merged commit a08fec6 into dotnet:master Feb 12, 2021

runfoapp bot mentioned this pull request Feb 12, 2021

Test Failure : System.Net.Security.Tests.SslStreamNetworkStreamTest.SslStream_ClientCertificate_SendsChain #48091

Closed

eiriktsarpalis mentioned this pull request Feb 16, 2021

PriorityQueue: Apply Comparer<T>.Default optimization #48346

Merged

DrewScoggins mentioned this pull request Feb 16, 2021

[Perf] Changes at 2/12/2021 4:56:39 PM DrewScoggins/performance-2#3977

Open

stephentoub mentioned this pull request Feb 16, 2021

Consider devirtualizing & inlining Comparer<T>.Default.CompareTo as well. #39873

Closed

EgorBo mentioned this pull request Feb 23, 2021

Fix C# warning in Comparer_get_Default #48643

Merged

ghost locked as resolved and limited conversation to collaborators Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[RyuJIT] Devirtualize Comparer<T>.Default#48160
EgorBo merged 9 commits intodotnet:masterfrom
EgorBo:jit-devirt-comparer-default

EgorBo commented Feb 11, 2021 •

edited

Loading

Uh oh!

stephentoub commented Feb 11, 2021

Uh oh!

stephentoub commented Feb 11, 2021

Uh oh!

AndyAyersMS left a comment

Uh oh!

Uh oh!

Uh oh!

EgorBo commented Feb 11, 2021

Uh oh!

AndyAyersMS commented Feb 11, 2021

Uh oh!

trylek left a comment

Uh oh!

eiriktsarpalis commented Feb 11, 2021

Uh oh!

AndyAyersMS commented Feb 12, 2021

Uh oh!

stephentoub commented Feb 12, 2021

Uh oh!

AndyAyersMS commented Feb 12, 2021

Uh oh!

akoeplinger commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

EgorBo commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Feb 11, 2021

Uh oh!

stephentoub commented Feb 11, 2021

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EgorBo commented Feb 11, 2021

Uh oh!

AndyAyersMS commented Feb 11, 2021

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

eiriktsarpalis commented Feb 11, 2021

master branch

PR branch

Uh oh!

AndyAyersMS commented Feb 12, 2021

Uh oh!

stephentoub commented Feb 12, 2021

Uh oh!

AndyAyersMS commented Feb 12, 2021

Uh oh!

akoeplinger commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EgorBo commented Feb 11, 2021 •

edited

Loading