Improve JitDisasm for top-level statements, expose DumpJittedMethods in Release by EgorBo · Pull Request #74090 · dotnet/runtime

EgorBo · 2022-08-17T18:39:07Z

For top-level statements (which are used by default when one creates console app via dotnet new console) JitDisasm wasn't easy to use for e.g.:

usingSystem;Foo();voidFoo()=>Console.WriteLine("Foo");

Both of these:

DOTNET_JitDisasm=Main
DOTNET_JitDisasm=Foo

didn't work as they had to be <Main>$ and <<Main>$>g__Foo|0_1() but now they should work as is.

Also, expose DOTNET_DumpJittedMethods=1 in Release so users can ask JIT to list all methods it's jitting - closes#74084
Example of its output:

JIT compiling 310 [Tier0]System.IO.StreamWriter::Write, ILsize=14
JIT compiling 311 [Tier0]System.String::op_Implicit, ILsize=31
JIT compiling 312 [Tier0]System.String::GetRawStringData, ILsize=7
JIT compiling 313 [Tier0]System.ReadOnlySpan`1[Char]::.ctor, ILsize=27
JIT compiling 314 [Tier0]System.IO.StreamWriter::WriteSpan, ILsize=368
JIT compiling 315 [Tier0]System.ReadOnlySpan`1[Char]::get_Item, ILsize=28
JIT compiling 316 [Tier0]System.Span`1[Byte]::Slice, ILsize=39
JIT compiling 317 [Tier0]System.Span`1[Byte]::.ctor, ILsize=27
JIT compiling 318 [Tier0]System.Span`1[Byte]::op_Implicit, ILsize=19
JIT compiling 319 [Tier0]System.ReadOnlySpan`1[Byte]::.ctor, ILsize=27
JIT compiling 320 [Tier0]WindowsConsoleStream::Write, ILsize=35
JIT compiling 321 [Tier0]WindowsConsoleStream::WriteFileNative, ILsize=107
JIT compiling 322 [Tier0]System.ReadOnlySpan`1[Byte]::get_IsEmpty, ILsize=10
JIT compiling 323 [Tier0]System.Threading.Thread::Sleep, ILsize=33
JIT compiling 324 [Tier1]System.Collections.HashHelpers::FastMod, ILsize=47
JIT compiling 325 [Tier1]System.Runtime.CompilerServices.CastHelpers::IsInstanceOfClass, ILsize=111
JIT compiling 326 [Tier1]System.Object::.ctor, ILsize=1
JIT compiling 327 [Tier1]System.ReadOnlySpan`1[Byte]::get_Length, ILsize=7
JIT compiling 328 [Tier1]System.Runtime.CompilerServices.RuntimeHelpers::GetMethodTable, ILsize=11
JIT compiling 329 [Tier1]System.ReadOnlySpan`1[Byte]::.ctor, ILsize=51
JIT compiling 330 [Tier1]System.Runtime.CompilerServices.RuntimeHelpers::IsReferenceOrContainsRef

I didn't expose neither JitOrder nor JitFunctionTrace as they look more low-level for end users, but happy to change that if you think it's worth exposing.

…se DumpJittedMethods for Release

ghost · 2022-08-17T18:39:22Z

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

Issue Details

For top-level statements (which are used by default when one creates console app via dotnet new console) JitDisasm wasn't easy to use for e.g.:

usingSystem;Foo();voidFoo()=>Console.WriteLine("Foo");

Both of these:

DOTNET_JitDisasm=Main
DOTNET_JitDisasm=Foo

didn't work as they had to be <Main>$ and <<Main>$>g__Foo|0_1() but now should work.

Also, expose DOTNET_DumpJittedMethods=1 in Release so users can ask JIT to list all methods it's jitting - closes #74084
Example of its output:

JIT compiling 310 [Tier0]System.IO.StreamWriter::Write, ILsize=14
JIT compiling 311 [Tier0]System.String::op_Implicit, ILsize=31
JIT compiling 312 [Tier0]System.String::GetRawStringData, ILsize=7
JIT compiling 313 [Tier0]System.ReadOnlySpan`1[Char]::.ctor, ILsize=27
JIT compiling 314 [Tier0]System.IO.StreamWriter::WriteSpan, ILsize=368
JIT compiling 315 [Tier0]System.ReadOnlySpan`1[Char]::get_Item, ILsize=28
JIT compiling 316 [Tier0]System.Span`1[Byte]::Slice, ILsize=39
JIT compiling 317 [Tier0]System.Span`1[Byte]::.ctor, ILsize=27
JIT compiling 318 [Tier0]System.Span`1[Byte]::op_Implicit, ILsize=19
JIT compiling 319 [Tier0]System.ReadOnlySpan`1[Byte]::.ctor, ILsize=27
JIT compiling 320 [Tier0]WindowsConsoleStream::Write, ILsize=35
JIT compiling 321 [Tier0]WindowsConsoleStream::WriteFileNative, ILsize=107
JIT compiling 322 [Tier0]System.ReadOnlySpan`1[Byte]::get_IsEmpty, ILsize=10
JIT compiling 323 [Tier0]System.Threading.Thread::Sleep, ILsize=33
JIT compiling 324 [Tier1]System.Collections.HashHelpers::FastMod, ILsize=47
JIT compiling 325 [Tier1]System.Runtime.CompilerServices.CastHelpers::IsInstanceOfClass, ILsize=111
JIT compiling 326 [Tier1]System.Object::.ctor, ILsize=1
JIT compiling 327 [Tier1]System.ReadOnlySpan`1[Byte]::get_Length, ILsize=7
JIT compiling 328 [Tier1]System.Runtime.CompilerServices.RuntimeHelpers::GetMethodTable, ILsize=11
JIT compiling 329 [Tier1]System.ReadOnlySpan`1[Byte]::.ctor, ILsize=51
JIT compiling 330 [Tier1]System.Runtime.CompilerServices.RuntimeHelpers::IsReferenceOrContainsRef

I didn't expose neither JitOrder nor JitFunctionTrace as they look more low-level for end users, but happy to change that if you think it's worth exposing.

Author:	EgorBo
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

jakobbotsch · 2022-08-17T19:51:56Z

JIT compiling 327 [Tier1]System.ReadOnlySpan`1[Byte]::get_Length, ILsize=7

Is this format compatible with JitDisasm? Shouldn't it be a single colon?

stephentoub · 2022-08-18T13:07:01Z

Can we backport this to rc1? From my perspective, it meets the bar of addressing significant gaps in a new feature. The support for DOTNET_JitDisasm is awesome (it's literally the first thing I talk about in the .NET 7 performance post I'm writing), but its usability is hard because there's no good way to discover what can be written as the value for the variable, and there's no good way to simply express names that get mangled by the C# compiler (e.g. async methods, iterators, local methods, top-level statements main, generated main for async mains, etc.). On top of that, being able to see (without separate tools) what methods the JIT is compiling at a high-level is a stellar pedagogical tool, but just specifying * leads to way too much spew for it to be useful; having the one-line-per-compilation is a great for understanding what's being generated when.

jakobbotsch · 2022-08-18T13:25:18Z

Just curious, but does it work to simply copy paste the name from DumpJittedMethods into JitDisasm to get the output in all cases? E.g. with generic instantiations, overloads... I feel like I often find that I cannot just copy the name directly. If @stephentoub is going to call these out in the blog post we should probably make sure this is seamless, or at least be aware of the cases that are broken.

I think my preference would be to make the matching follow some very basic understandable rules, e.g. something like: the JitDisasm string is a space separated list of patterns. If any pattern matches the method has its disassembly printed. Patterns can contain wildcards and characters. The string matched against depends on the pattern, but starts out with just the method name:

If the pattern contains (, then the method string name additionally contains its signature at the end (in the same format JitDisasm/DumpJittedMethods shows)
If the pattern contains :, then the method name string starts with ClassName: (again, compatible with the strings displayed by JitDisasm/DumpJittedMethods)

I'm not sure how this matches the rules today.

EgorBo · 2022-08-18T16:33:45Z

@jakobbotsch I've just checked - whatever DOTNET_DumpJittedMethods=1 prints can be used for JitDisasm just fine with two special cases:

If you use powershell you might need to escape ` symbol (works in cmd)
VM's eeGetMethodFullName behaves differently for Release and Debug, it doesn't print generic signature in Release and I don't want to change that now (especially if we plan to backport this) 🙂 - so e.g. instead of Foo'1[Cannon] it prints just Foo'1 - so you might see some problems if you use Checked's DOTNET_DumpJittedMethods in Release's DOTNET_JitDisasm

So, to match CastHelpers:StelemRef(Array,long,Object) we currently can use the following patterns (some of them will print other methods too):

DOTNET_JitDisasm=CastHelpers:StelemRef(Array,long,Object)
DOTNET_JitDisasm=CastHelpers:StelemRef
DOTNET_JitDisasm=StelemRef
DOTNET_JitDisasm=StelemRef*
DOTNET_JitDisasm=StelemR*
DOTNET_JitDisasm=*telemRe*
DOTNET_JitDisasm=*
DOTNET_JitDisasm=**
DOTNET_JitDisasm=CastHelpers:*
DOTNET_JitDisasm=CastHelpers:**
DOTNET_JitDisasm=CastHelpers:*telemRe*
DOTNET_JitDisasm=CastHelpers::*
DOTNET_JitDisasm=CastHelpers::**
DOTNET_JitDisasm=CastHelpers::*telemRe*

Wildcards only work for methodName, className can be omitted or exactly match.

@stephentoub do you have any preferences for better name instead of DOTNET_DumpJittedMethods=1 ? e.g. DOTNET_JitDisasmList=1 etc

stephentoub · 2022-08-18T17:05:30Z

@stephentoub do you have any preferences for better name instead of DOTNET_DumpJittedMethods=1 ? e.g. DOTNET_JitDisasmList=1 etc

No strong preference. DOTNET_JitListMethods? DOTNET_JitListCompilations? DOTNET_JitDisasmSummary?

EgorBo · 2022-08-19T12:59:24Z

@dotnet/jit-contrib PTAL, it's ready for review - also, any thoughts on a better name for end users? 🙂

TIHan · 2022-08-19T16:54:16Z

 CONFIG_METHODSET(JitDisasm, W("JitDisasm"))
 #endif // !defined(DEBUG)

+CONFIG_INTEGER(DumpJittedMethods, W("DumpJittedMethods"), 0) // Prints all jitted methods to the console


This is going to be awesome

BruceForstall

As to naming, I prefer Stephen's suggestion DOTNET_JitDisasmSummary

BruceForstall · 2022-08-19T17:35:56Z

Can you show some examples of the newest output format, especially including some OSR methods (e.g., from the ASP.NET collection)?

Co-authored-by: Bruce Forstall <brucefo@microsoft.com>

BruceForstall · 2022-08-19T22:14:26Z

Can you show an example with OSR in a Checked JIT? With JitStress enabled (in Checked)?

Are you going to rename the option?

AndyAyersMS · 2022-08-19T23:00:37Z

Would also be nice to know if we're generating instrumented code and (perhaps) if we have access to PGO data -- and if so, what kind (at least for the root).

kunalspathak · 2022-08-20T00:33:36Z

 {
- // 's2' must start with 'nameLen' characters of 'name'
- if (strncmp(name, s2, nameLen) != 0)
+ for (size_t srcPos = 0; srcPos <= srcSize - needleSize; srcPos++)


assert(srcSize >= needleSize)?

if needleSize is bigger it will just return "not found" because it actually can be bigger

But in that case won't the srcSize - needleSize way bigger? You might just fast return "not found" in such cases.

Guess this is still unaddressed.

@kunalspathak sorry didn't notice your comment - I am not sure I understand, let's say
srcSize is 10 and needleSize is 20, then srcSize - needleSize is -10 so for-loop immediately exits and returns "not found" - am I wrong?

I haven't looked in depth here, but srcSize - needleSize can never be negative, since size_t is unsigned.

ah oops, it supposed to be ssize_t

I haven't looked in depth here, but srcSize - needleSize can never be negative, since size_t is unsigned.
^ That.

AndyAyersMS · 2022-08-20T01:16:20Z

6: JIT compiled Proga:Main() [Tier1-OSR, IL size=27, code size=56]

We should make sure the patchpoint IL offset shows up here.

BruceForstall · 2022-08-20T04:51:51Z

6: JIT compiled Proga:Main() [Tier1-OSR, IL size=27, code size=56]
We should make sure the patchpoint IL offset shows up here.

It looks like the code to do that is only under ifdef DEBUG but doesn't need to be.

In fact, the "DEBUG" version, including compGetStressMessage(), should be used for Release (for simplicity) since compGetStressMessage() already ifdefs for DEBUG.

EgorBo · 2022-08-20T10:12:23Z

Unified Debug and Release, initially I just didn't want to show users irrelevant data they won't ever need

New Release output:

 1: JIT compiled CastHelpers:StelemRef(Array,long,Object) [Tier1, IL size=88, code size=93, hash=0x00000000 ]
2: JIT compiled CastHelpers:LdelemaRef(Array,long,long):byref [Tier1, IL size=44, code size=44, hash=0x00000000 ]
3: JIT compiled SpanHelpers:IndexOfNullCharacter(byref):int [Tier1, IL size=792, code size=388, hash=0x00000000 ]
4: JIT compiled Proga:Main() [Tier0, IL size=42, code size=124, hash=0x00000000 ]
5: JIT compiled Proga:foo() [Tier0, IL size=1, code size=6, hash=0x00000000 ]
6: JIT compiled Proga:foo() [Tier1, IL size=1, code size=1, hash=0x00000000 ]
7: JIT compiled Proga:Main() [Tier1-OSR @0x21, IL size=42, code size=56, hash=0x00000000

OSR offset is here.

Also, renamed to DOTNET_JitDisasmSummary

@BruceForstall

BruceForstall

LGTM with one small request.

Andy said:

Would also be nice to know if we're generating instrumented code and (perhaps) if we have access to PGO data -- and if so, what kind (at least for the root).

Don't know if you want to address that, and if so, in this change or as a follow-up.

Co-authored-by: Bruce Forstall <brucefo@microsoft.com>

EgorBo · 2022-08-22T15:00:42Z

Added PGO on @AndyAyersMS's request, example:

 33: JIT compiled SubStringCountTest:Foo():int [Instrumented Tier0, IL size=29, code size=156, hash=0x00000000]
34: JIT compiled SubStringCountTest:Foo():int [Instrumented Tier1-OSR @0x13 with Dynamic PGO, IL size=29, code size=63, hash=0x00000000]
35: JIT compiled SubStringCountTest:Foo():int [Tier1 with Dynamic PGO, IL size=29, code size=22, hash=0x00000000]
36: JIT compiled Thread:Sleep(int) [Tier1 with Static PGO, IL size=33, code size=148, hash=0x00000000]

e.g. 35 is both instrumented and optimized with PGO due to OSR

BruceForstall · 2022-08-22T17:10:56Z

Given that hash is always zero in Release, maybe we shouldn't display it in Release builds?

EgorBo · 2022-08-22T23:45:05Z

/backport to release/7.0-rc1

github-actions · 2022-08-22T23:45:19Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2907852964

github-actions · 2022-08-22T23:47:19Z

@EgorBo an error occurred while backporting to release/7.0-rc1, please check the run log for details!

Error: @EgorBo is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=EgorBo

EgorBo · 2022-08-22T23:49:50Z

/backport to release/7.0-rc1

github-actions · 2022-08-22T23:50:04Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2907874603

github-actions · 2022-08-22T23:52:23Z

@EgorBo backporting to release/7.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Improve JitDisasm for top-level statements, nested functions and expo…
caf1dcf
…se DumpJittedMethods for Release

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 17, 2022

ghost assigned EgorBo Aug 17, 2022

jakobbotsch reviewed Aug 17, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/jitconfig.cpp Outdated

jkotas reviewed Aug 17, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

EgorBo added 2 commits August 18, 2022 00:51

Wildcards support
bc37fd9

Address feedback
859209c

jkotas reviewed Aug 18, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

SingleAccretion reviewed Aug 18, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

EgorBo added 3 commits August 18, 2022 10:58

Address feedback
cbcadf5

Update compiler.cpp
02b97e5

fix gcc
d49f397

Clean up
b52364a

JulieLeeMSFT added this to the 8.0.0 milestone Aug 18, 2022

TIHan reviewed Aug 19, 2022
View reviewed changes

BruceForstall suggested changes Aug 19, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 19, 2022

BruceForstall reviewed Aug 19, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

Update src/coreclr/jit/compiler.cpp
08ae661
Co-authored-by: Bruce Forstall <brucefo@microsoft.com>

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 19, 2022

EgorBo added 2 commits August 19, 2022 23:11

Clean up
8767829

Merge branch 'main' of github.com:dotnet/runtime into improve-jitdisasm
6a397f3

kunalspathak reviewed Aug 20, 2022
View reviewed changes

kunalspathak mentioned this pull request Aug 20, 2022
Spmi-diff is showing diffs that are not related to the PR #74287
Closed

EgorBo added 2 commits August 20, 2022 12:14

Address feedback
4ec9cb1

Merge branch 'main' of github.com:dotnet/runtime into improve-jitdisasm
f6a6ffd

BruceForstall approved these changes Aug 20, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.cpp Outdated

EgorBoand others added 2 commits August 22, 2022 14:31

Update src/coreclr/jit/compiler.cpp
77e1e89
Co-authored-by: Bruce Forstall <brucefo@microsoft.com>

Add PGO/Instrumentation notes
691d7d3

add "with"
9658c2d

AndyAyersMS approved these changes Aug 22, 2022
View reviewed changes

address feedback
2c6c231

BruceForstall approved these changes Aug 22, 2022
View reviewed changes

EgorBo merged commit d80d71f into dotnet:mainAug 22, 2022

EgorBo removed their assignment Aug 23, 2022

This was referenced Aug 23, 2022
[release/7.0-rc1] Improve DOTNET_JitDisasm and introduce DOTNET_JitDisasmSummary #74392
Merged
Fix strnstr in jitconfig.cpp #74421
Merged

EgorBo deleted the improve-jitdisasm branch August 23, 2022 12:37

ghost locked as resolved and limited conversation to collaborators Sep 22, 2022

Uh oh!

Conversation

EgorBo commented Aug 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 17, 2022

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Aug 17, 2022

Uh oh!

Uh oh!

Uh oh!

stephentoub commented Aug 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch commented Aug 18, 2022

Uh oh!

EgorBo commented Aug 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Aug 18, 2022

Uh oh!

EgorBo commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BruceForstall commented Aug 19, 2022

Uh oh!

Uh oh!

BruceForstall commented Aug 19, 2022

Uh oh!

AndyAyersMS commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Aug 20, 2022

Uh oh!

BruceForstall commented Aug 20, 2022

Uh oh!

EgorBo commented Aug 20, 2022

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EgorBo commented Aug 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceForstall commented Aug 22, 2022

Uh oh!

EgorBo commented Aug 22, 2022

Uh oh!

github-actionsBot commented Aug 22, 2022

Uh oh!

github-actionsBot commented Aug 22, 2022

Uh oh!

EgorBo commented Aug 22, 2022

Uh oh!

github-actionsBot commented Aug 22, 2022

Uh oh!

github-actionsBot commented Aug 22, 2022

EgorBo commented Aug 17, 2022•
edited
Loading

stephentoub commented Aug 18, 2022•
edited
Loading

EgorBo commented Aug 18, 2022•
edited
Loading

EgorBo commented Aug 22, 2022•
edited
Loading