Add a config to MIBC format #71359

EgorBo · 2022-06-28T00:28:10Z

This PR adds the following global method to *.mibc - MibcConfig that stores key-value config like this (similar to AssemblyDictionary method):

When we merge multiple mibc we just take the first valid config for the merged mibc.

When we do "dotnet-pgo dump" it's displayed like this:

FormatVersion:     1.0
Os:                Windows
Arch:              x64
Runtime:           CoreClr

# Methods: 1382
# Methods with any profile data: 677
# Methods with 32-bit block counts: 0
# Methods with 64-bit block counts: 677
# Methods with 32-bit edge counts: 0
# Methods with 64-bit edge counts: 0
# Type handle histograms: 522 in 209 methods
# GetLikelyClass data: 0 in 0 methods
# Call sites where getLikelyClass is null: 253
# Call sites where getLikelyClass is unknown: 0
# Call sites where getLikelyClass returns data that devirtualizes: 269

Call-sites grouped by number of likely classes seen:

          0: [##########..........] 48.5% (253) - call-sites with 'null's
          1: [##########..........] 50.2% (262) - monomorphic call-sites
          2: [....................]  1.3% (7)
          3: [....................]  0.0% (0)
          4: [....................]  0.0% (0)
          5: [....................]  0.0% (0)
          6: [....................]  0.0% (0)
          7: [....................]  0.0% (0)
          8: [....................]  0.0% (0)
          9: [....................]  0.0% (0)
        ≥10: [....................]  0.0% (0)


Likelihoods of the most popular likely classes:

  [ 0- 10%]: [##########..........] 253
  (10- 20%]: [##..................] 47
  (20- 30%]: [....................] 9
  (30- 40%]: [#...................] 14
  (40- 50%]: [....................] 4
  (50- 60%]: [....................] 0
  (60- 70%]: [....................] 0
  (70- 80%]: [#...................] 39
  (80- 90%]: [....................] 7
  (90-100%]: [######..............] 149

Generated pgo2.mibc.txt

EgorBo · 2022-06-28T00:51:03Z

Let me know if I e.g. should serialize it to e.g. JSON or CSV and put into a single ldstr or if there is a way to write the whole thing to a sort of manifest and stuff like that.

MichalStrehovsky · 2022-06-28T01:25:27Z

One could also serialize this info as custom attributes. The ldstr/pop and ldtoken/pop is used for the rest of the MIBC stuff because it's more compact than custom attributes. We don't care about compactness for metadata that much. Custom attributes look nicer when looked at in e.g. ilspy. One could use AssemblyMetadataAttribute from mscorlib so that it even nicely resolves to something.

But not sure if it's a scenario we need to optimize for and you already wrote the code to use ldstr/pop...

src/coreclr/tools/dotnet-pgo/Program.cs

mdh1418 · 2022-06-28T14:11:17Z

src/coreclr/tools/dotnet-pgo/Program.cs

+                    config.Arch = processInfo?.PayloadByName("ArchInformation")?.ToString();
+
+                    // Look for Sku, e.g. "CoreClr"
+                    TraceEvent runtimeStart = p.EventsInProcess.Filter(t => t.EventName == "Runtime/Start").FirstOrDefault();


How does the Sku work (what does it stand for)? It would be nice to have the mono runtime be emitted, so if we use the Sku for emitting runtime, would it be unproblematic to extend the Sku enum to also contain Mono (not sure if there is a trickle effect)? What is the Runtime/Start event/where do i look to find it?

I wasn't sure what to pick for runtime so probably it would be easier if you tell me which event to analyze cc @lateralusX
I just opened a random nettrace via PerfView and searched for CoreCLR - this was the first thing to pop up - I assume mono runtime is expected to send this exact event on start with a different value for Sku

If that eventname is the same as FireEtwRuntimeInformationDCStart, it seems to be the right event. I think we can try adding mono to the Sku Enum, right now we are emitting CoreCLR as well on Mono

runtime/src/mono/mono/eventpipe/ep-rt-mono.c

Line 2804 in 41d9c1c

RUNTIME_SKU_CORECLR,

since it wasn't clear whether a non 0x1, 0x2 Sku value will cause problems anywhere that its used.

I don't see FireEtwRuntimeInformationDCStart in my sample nettrace

Here is the events we collect - Microsoft-Windows-DotNETRuntime:0x1F000080018:5

So I assume the current impl is correct as is, right? it does poll "Runtime/Start" event and extract Sku so once Mono is added we're all set

@lateralusX @EgorBo I'm thinking the value should be 0x4 as 0x3 will show up as DesktopClr, CoreClr since it matches those bits (0x1 || 0x2). I obtained a trace with --providers Microsoft-Windows-DotNETRuntime:0x1F000080018:5 on mono and it indeed shows the sku value within the mibc file. https://gist.github.com/mdh1418/2e9ad2d937dbdd0fb0be063c15eff61a

I also verified that we should be able to obtain the actual Sku value from the Mono AOT Compiler side as well. I'll put up a separate PR to integrate the .mibc file validation.

I think we should extend the relevant Sku structs and Mono additions I mentioned earlier

Is anything treating the sku as a bit set? Seems like it shouldn't be.

@jakobbotsch I'm not sure what the main rationale is between making something a bitmap vs a valuemap, but this might be why its behaving as such

runtime/src/coreclr/vm/ClrEtwAll.man

Line 611 in 4a63cb2

<bitMap name="RuntimeSkuMap">

I see, seems reasonable then.

src/coreclr/tools/dotnet-pgo/MibcConfig.cs

src/coreclr/tools/dotnet-pgo/Program.cs

jakobbotsch

LGTM, couple of nits you can consider

mdh1418

I realized I forgot to submit the comments so all of my comments were pending 😅

src/coreclr/tools/aot/ILCompiler.ReadyToRun/IBC/MIbcProfileParser.cs

mdh1418 · 2022-06-29T18:17:57Z

src/coreclr/tools/dotnet-pgo/Program.cs

+                    config.Arch = processInfo?.PayloadByName("ArchInformation")?.ToString();
+
+                    // Look for Sku, e.g. "CoreClr"
+                    TraceEvent runtimeStart = p.EventsInProcess.Filter(t => t.EventName == "Runtime/Start").FirstOrDefault();


I extended the Sku Enum to have Mono 0x4 in both src/coreclr/inc/eventtracebase.h and src/coreclr/nativeaot/Runtime/eventtracebase.h

typedef enum _Sku { DesktopCLR=0x1, CoreCLR=0x2, Mono=0x4 }Sku;

and the generated .nettrace is still compatible with perfview and visual studio (as in no throws or crashes from viewing the .nettrace). The only catch is that the value of Sku is literal Sku="4" compared to Sku="CoreClr" and Sku="DesktopClr". I believe that is because the libraries define the Sku as RuntimeSku with only enum vars for 0x1 and 0x2. https://github.com/microsoft/perfview/blob/8f5c46ad3980ecbd8845bff274911ecb22f98f3f/src/TraceEvent/Parsers/ClrEtwAll.cs.base#L6902-L6906

I'm not sure if there are other tooling or more advanced usages of .nettrace Rundown Runtime/Start events that leverage the value of Sku, but if this is the only extent, then it looks like we are good to go to with using the event.

@EgorBo can we include extending the Sku in this PR?

typedef enum _Sku { DesktopCLR=0x1, CoreCLR=0x2, Mono=0x4 }Sku;

in both src/coreclr/inc/eventtracebase.h and src/coreclr/nativeaot/Runtime/eventtracebase.h
and changing the
#define RUNTIME_SKU_CORECLR 0x2 to #define RUNTIME_SKU_MONO 0x4

runtime/src/mono/mono/eventpipe/ep-rt-mono.c

Line 153 in ade0b21

#define RUNTIME_SKU_CORECLR 0x2

as well as the rundown event Sku value

runtime/src/mono/mono/eventpipe/ep-rt-mono.c

Line 2804 in ade0b21

RUNTIME_SKU_CORECLR,

to be RUNTIME_SKU_MONO

mdh1418 · 2022-07-01T17:11:58Z

src/coreclr/tools/dotnet-pgo/Program.cs

+                    config.Arch = processInfo?.PayloadByName("ArchInformation")?.ToString();
+
+                    // Look for Sku, e.g. "CoreClr"
+                    TraceEvent runtimeStart = p.EventsInProcess.Filter(t => t.EventName == "Runtime/Start").FirstOrDefault();


@lateralusX @EgorBo I'm thinking the value should be 0x4 as 0x3 will show up as DesktopClr, CoreClr since it matches those bits (0x1 || 0x2). I obtained a trace with --providers Microsoft-Windows-DotNETRuntime:0x1F000080018:5 on mono and it indeed shows the sku value within the mibc file. https://gist.github.com/mdh1418/2e9ad2d937dbdd0fb0be063c15eff61a

I also verified that we should be able to obtain the actual Sku value from the Mono AOT Compiler side as well. I'll put up a separate PR to integrate the .mibc file validation.

I think we should extend the relevant Sku structs and Mono additions I mentioned earlier

lateralusX · 2022-07-04T07:21:32Z

@mdh1418 We should extend ClrEtwAll.man with the new enum values as well as strings. Since this PR has been merged, we need do it in separate PR.

Once we have the ClrEtwAll.man extended, we should also make sure perfview and EventTrace library correctly handles the new enum value. @brianrob can guide you through the steps needed to update these libraries in order to align to extensions to ClrEtwAll.man.

Add MibcConfig to mibc

8319fa8

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Jun 28, 2022

ghost assigned EgorBo Jun 28, 2022

EgorBo mentioned this pull request Jun 28, 2022

Include runtime flavor and platform information into .mibc Profile #71065

Closed

EgorBo requested review from davidwrighton and jakobbotsch and removed request for davidwrighton June 28, 2022 13:58

mdh1418 reviewed Jun 28, 2022

View reviewed changes

jakobbotsch reviewed Jun 28, 2022

View reviewed changes

src/coreclr/tools/dotnet-pgo/MibcConfig.cs Outdated Show resolved Hide resolved

jakobbotsch reviewed Jun 28, 2022

View reviewed changes

src/coreclr/tools/dotnet-pgo/Program.cs Outdated Show resolved Hide resolved

jakobbotsch approved these changes Jun 28, 2022

View reviewed changes

EgorBo added 5 commits June 28, 2022 20:17

Address feedback

3a8f82d

Clean up

0b68488

Fix build

1714407

Fix restore

6ebfbcb

fix crash

11cab7a

EgorBo closed this Jun 28, 2022

EgorBo reopened this Jun 28, 2022

EgorBo added 2 commits June 30, 2022 14:55

Merge branch 'main' of github.com:dotnet/runtime into extend-mibc

776061a

Get rid of reflection

285eab6

mdh1418 reviewed Jul 1, 2022

View reviewed changes

EgorBo added 2 commits July 1, 2022 22:05

Merge branch 'main' of github.com:dotnet/runtime into extend-mibc

03beced

Address feedback

1cf0aef

EgorBo requested review from lambdageek and MichalStrehovsky as code owners July 1, 2022 20:12

davidwrighton approved these changes Jul 1, 2022

View reviewed changes

mdh1418 approved these changes Jul 1, 2022

View reviewed changes

EgorBo merged commit 9b10634 into dotnet:main Jul 1, 2022

This was referenced Jul 5, 2022

Integrate mibc mono config extension #71657

Merged

[TraceEvent] Extend RuntimeSku to recognize Mono microsoft/perfview#1658

Merged

ghost locked as resolved and limited conversation to collaborators Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a config to MIBC format #71359

Add a config to MIBC format #71359

EgorBo commented Jun 28, 2022 •

edited

Loading

EgorBo commented Jun 28, 2022

MichalStrehovsky commented Jun 28, 2022

mdh1418 Jun 28, 2022

EgorBo Jun 28, 2022

mdh1418 Jun 28, 2022

EgorBo Jun 28, 2022

EgorBo Jun 28, 2022

EgorBo Jun 30, 2022 •

edited

Loading

mdh1418 Jul 1, 2022

jakobbotsch Jul 1, 2022

mdh1418 Jul 8, 2022

jakobbotsch Jul 8, 2022

jakobbotsch left a comment

mdh1418 left a comment

mdh1418 Jun 29, 2022

mdh1418 Jul 1, 2022

lateralusX commented Jul 4, 2022 •

edited

Loading

Add a config to MIBC format #71359

Add a config to MIBC format #71359

Conversation

EgorBo commented Jun 28, 2022 • edited Loading

EgorBo commented Jun 28, 2022

MichalStrehovsky commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

mdh1418 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateralusX commented Jul 4, 2022 • edited Loading

EgorBo commented Jun 28, 2022 •

edited

Loading

EgorBo Jun 30, 2022 •

edited

Loading

lateralusX commented Jul 4, 2022 •

edited

Loading