JIT: Enhance metrics reported by superpmi diffs #74584

jakobbotsch · 2022-08-25T14:54:31Z

Report the total number of contexts, minopts contexts and fullopts
contexted processed
Report number of successful and missing contexts
Report asmdiffs and tpdiffs for minopts/fullopts separately

Fixes #70350
Contributes to #73506

* Report the total number of contexts, minopts contexts and fullopts contexted processed * Report number of successful and missing contexts * Report asmdiffs and tpdiffs for minopts/fullopts separately Fixes dotnet#70350 Contributes to dotnet#73506

ghost · 2022-08-25T14:54:59Z

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

Issue Details

Report the total number of contexts, minopts contexts and fullopts
contexted processed
Report number of successful and missing contexts
Report asmdiffs and tpdiffs for minopts/fullopts separately

Fixes #70350
Contributes to #73506

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

PAL does not have the template variant of vsprintf_s.

* Rename Total -> Overall, Opts -> FullOpts * Move Overall column to the right * Adjust some colors * Include minipal/utils.h so we can use ARRAY_SIZE, which hopefully fixes the build on non-Windows.

…mi-metrics

jakobbotsch · 2022-09-13T19:53:20Z

@dotnet/jit-contrib I'm decently happy with what I have now. It looks like the following for a change with diffs: https://dev.azure.com/dnceng-public/public/_build/results?buildId=14828&view=ms.vss-build-web.run-extensions-tab

Currently running it to make sure it does not look horrible when there are no diffs, but otherwise this is ready for review. @kunalspathak can you take a look?

kunalspathak

More details are always good to have, but I honestly think that the extension page is getting very crowded, and it is hard to follow the most common information that we want to see.

I think the confusing part is when you extend "Details" and at one point I lose track of whose details I was looking for.

kunalspathak · 2022-09-14T02:34:19Z

src/coreclr/scripts/superpmi_diffs_summarize.py

@@ -89,39 +89,8 @@ def append_diff_file(f, arch, file_name, full_file_path, asmdiffs):
        else:
            diffs_found = True
            f.write("## {} {}\n".format(diff_os, diff_arch))
-


Can you remind me why this section is removed?

The sections are printed by superpmi.py now.

kunalspathak · 2022-09-14T02:40:23Z

src/coreclr/scripts/superpmi.py

+                    color = "red" if diff > base else "green"
+                    return "<span style=\"color:{}\">{}{:,d}</span>".format(color, plus_if_positive, diff - base)
+
+                diffed_contexts = sum(int(diff_metrics["Overall"]["Successful compiles"]) for (_, _, diff_metrics, _, _) in asm_diffs)


can you convert sum(....) into a function where you pass 1st and 2nd level names? E.g. get_sum("Overall", "Successful compiles")?

kunalspathak · 2022-09-14T02:42:50Z

src/coreclr/scripts/superpmi.py

+
+                        write_fh.write("<summary>{} ({} bytes)</summary>\n\n".format(col, format_delta(sum_base, sum_diff)))
+                        write_fh.write("|Collection|Base size (bytes)|Diff size (bytes)|\n")
+                        write_fh.write("|---|--:|--:|\n")


Is it possible to draw table and cell boundaries to see things clearly? I have to stare at them to understand which number belongs to which column.

I'm not sure if this is possible with markdown tables in Azure Pipelines. I agree the default style there is not great, though with only three columns I haven't really had this particular problem.
Maybe switching to HTML tables would work.

kunalspathak · 2022-09-14T02:52:30Z

src/coreclr/tools/superpmi/superpmi/jitinstance.cpp

+        *pParam->isMinOpts =
+            jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE) ||
+            jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT) ||
+            jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0);


I don't think Tier0 code is same as MinOpt.

Today they are practically the same, I think. They both lead to OptimizarionDisabled() returning true.

(Also note that Compiler::Options::MinOpts() returns true in tier-0 compilations, so it's not wrong to say that tier-0 compilations are MinOpts)

I see, I was checking yesterday and comparing it with "debuggable code" which is different. I was thinking that "debuggable code" is min-opts.

jakobbotsch · 2022-09-14T07:37:54Z

More details are always good to have, but I honestly think that the extension page is getting very crowded, and it is hard to follow the most common information that we want to see.

What is the most common information you want to see? Currently it is showing the overall diffs for each collection by default, which is what I want to see, but required multiple clicks before to individually look at.

I do agree it is still a bit overwhelming to look at, but I'm not sure how to present it better. Part of this is that some of this information is new but is information we really ought to always have looked at as part of evaluating changes (in particular the MISSED contexts).

jakobbotsch · 2022-09-14T08:46:01Z

the extension page is getting very crowded

Do you think it would be better if I leave the "Overall" sections unexpanded by default? Then you at least aren't presented with so many tables immediately when you go to look.

This reverts commit e58e404.

kunalspathak · 2022-09-14T13:37:49Z

Do you think it would be better if I leave the "Overall" sections unexpanded by default? Then you at least aren't presented with so many tables immediately when you go to look.

Yes, that's what I mean by important information shown by default and everything else is hidden.

jakobbotsch · 2022-09-14T13:42:44Z

Yes, that's what I mean by important information shown by default and everything else is hidden.

Well, that's already how it was. It showed you the diffs by default and hid everything else.

The new one that hides the diffs by default looks like this: https://dev.azure.com/dnceng-public/public/_build/results?buildId=16291&view=ms.vss-build-web.run-extensions-tab

Since we show the sum of diffs now it seems fine to me to hide the per-collection diffs by default. What do you think?

I've also changed the Details section a bit by adding a horizontal line between the table and jit-analyze output (which I have added a header for as well).

The "sum of diffs" is not really a meaningful number to base anything on, but it does help to guide where one should drill in.

kunalspathak · 2022-09-14T14:02:09Z

I mean, here I am not sure showing > MinOpts (+0 bytes), > FullOpts (+194,496 bytes) by default is of any value. It is good information to have, but not be default IMO. We can just stick with Overall numbers.

If someone sees those numbers, they anyway don't have clue when they expand "Details diff" to see where they are coming from? I would suggest putting it few clicks under should be fine. For instance, we might not have cases where MinOpts code size diffs are changing frequently, but perhaps they might show up once in a while on TP metrics.

The "sum of diffs" is not really a meaningful number to base anything on, but it does help to guide where one should drill in.

How about removing the "Base size" column? What are we going to do knowing that? Instead `Diff size (bytes)` and `Diff %` might be better?

I always felt spacing between Details and next configuration say OSX arm64 show be more to distinguish that we are starting the next section. Without that it looks more congested when expanded.

Probably a fancy line separator might help.

If we are ever able to post back these results to PR as comments, I would like to include some asmdiff examples of each category too and then, we will have to find some more space to have them.

Again, these are just some suggestions, and it might take multiple rounds to make it to the point where it proves readable for everyone. Feel free to ignore some of the suggestions.

jakobbotsch · 2022-09-14T15:02:06Z

I mean, here I am not sure showing > MinOpts (+0 bytes), > FullOpts (+194,496 bytes) by default is of any value. It is good information to have, but not be default IMO. We can just stick with Overall numbers.

I strongly disagree. Having the diffs separated out into these categories is quite important to me. Code size regressions in MinOpts are not as important as code size regressions in FullOpts. TP regressions in MinOpts are more important than TP regressions in FullOpts. If I'm making a transformation that is enabled only when optimizing then "Overall" is a misleading category -- I am interesting in its impact only on optimized contexts. I am also interested in verifying that there is no impact on MinOpts.

FWIW, I recall seeing quite a few instances of "small diffs except in the asp.net collection", which I always viewed as an indication that there were primarily MinOpts diffs in a change.

I'd be fine with hiding it under "Details", but I think the separated categories are quite important. What does the rest of @dotnet/jit-contrib think? Should they be shown by default or hidden away?

If someone sees those numbers, they anyway don't have clue when they expand "Details diff" to see where they are coming from?

We can fix jit-analyze separately. At least this makes it clear that there is impact on these categories of contexts and that further investigation is warranted.

How about removing the "Base size" column? What are we going to do knowing that? Instead Diff size (bytes) and Diff % might be better?

This was how I first had it, but I quickly saw that "Diff %" is almost always too small to give any meaningful information. For most changes I looked at it is 0.00%. This very large forward sub change is in the 0.01% - 0.02% range. So this was an alternative way to give some notion of relative impact. Not sure if there's a better way (permille?) -- I personally have internalized what a "large diff" is based on just the absolute diff after working on the JIT for a while.

I always felt spacing between Details and next configuration say OSX arm64 show be more to distinguish that we are starting the next section. Without that it looks more congested when expanded.

I agree, let me try to add some more separation.

SingleAccretion · 2022-09-14T16:16:27Z

Should they be shown by default or hidden away?

I agree with Jakob that the minopts/fullopts separation should be shown prominently.

I also agree it's a bit unfortunate getting to the jit-analyze output is not so quick, since it contain the outliers (large relative regressions), but I don't think it's a big deal.

Suggestion: grey out (don't color) 0.00% for TP diffs - green -0.00% look a bit strange.

kunalspathak · 2022-09-14T18:45:30Z

Code size regressions in MinOpts are not as important as code size regressions in FullOpts. TP regressions in MinOpts are more important than TP regressions in FullOpts.

That's exactly what I stated: "For instance, we might not have cases where MinOpts code size diffs are changing frequently, but perhaps they might show up once in a while on TP metrics." and if we agree that why not hide code size diffs of MinOpts and show TP diff instead by default?

I think I am trying to convey that we should just surface minimum information on top that matters 80% of time and rest all can be hidden behind few clicks.

jakobbotsch · 2022-09-15T09:35:43Z

if we agree that why not hide code size diffs of MinOpts and show TP diff instead by default?

So is your suggestion to move the MinOpts section under Details for asmdiffs, and the FullOpts section under Details for the TP diffs? But retain Overall/FullOpts and OverAll/MinOpts respectively? It's odd to me to have these sections appear in different places.

FWIW, I agree the diffs themselves are less important if they are expected. But easily being able to see whether there are any unexpected diffs is very important too. I dislike having to go expand various subsections for something that I expect I always want to check.

kunalspathak · 2022-09-15T13:04:18Z

Sounds ok to me. May be adding the extra spacing around (as I suggested above) might make it better. And of course, we can re-iterate with better ideas.

AndyAyersMS · 2022-09-16T17:35:42Z

we can re-iterate with better ideas.

Right, this is a clear improvement over what we have now, and once we have some mileage on it we can revise.

My only suggestion would be to try and also record the net +/- size contributions, so it is easier to tell if something is a pure improvement or regression or mixed bag, eg

Overall (+190,456 bytes) (+250,000/-59,544)

But this can wait. Maybe even in the fullness of time, some sort of empirical PDF/histogram for the gains and losses. I am usually interested in knowing when there are outliers and such... many of my recent changes have a lot of small wins and a few big regressions, for example.

This reverts commit a710c45.

jakobbotsch · 2022-09-19T13:11:29Z

This is how it looks with no diffs: https://dev.azure.com/dnceng-public/public/_build/results?buildId=21852&view=ms.vss-build-web.run-extensions-tab
Unfortunately we have spurious diffs in the coreclr_tests.run collection for arm platforms right now (see #75672 (comment))). But you can see how it should look with no diffs for the Linux/Windows x64 sections.

I think I'll remove the "missed contexts" line when there are no misses, though it's probably a quite rare situation.

jakobbotsch added 2 commits August 25, 2022 16:51

Add some artificial diffs

706953d

ghost assigned jakobbotsch Aug 25, 2022

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 25, 2022

Fix PAL build

4ad92a1

PAL does not have the template variant of vsprintf_s.

JulieLeeMSFT added this to the 8.0.0 milestone Aug 25, 2022

jakobbotsch added 13 commits August 25, 2022 21:46

Get some diffs in release

13a4ab7

Remove percentage, try some color

75f71a0

Various adjustments

631d489

* Rename Total -> Overall, Opts -> FullOpts * Move Overall column to the right * Adjust some colors * Include minipal/utils.h so we can use ARRAY_SIZE, which hopefully fixes the build on non-Windows.

Reorder/rename some columns

c8d4b8e

Separate sections for overall/minopts/fullopts

5d79fb9

Merge branch 'main' of github.com:dotnet/runtime into spmi-metrics

70c4abd

Fixes

e7087df

Merge branch 'spmi-metrics' of github.com:jakobbotsch/runtime into sp…

286a1f5

…mi-metrics

Optimized -> FullOpts, switch some wording, switch back to bytes

654a5d9

Right align numbers

365a0d4

Change some headings

38a85fb

Handle JIT switching to minopts/fullopts

366523a

Revert forward sub change

e58e404

jakobbotsch marked this pull request as ready for review September 13, 2022 19:53

jakobbotsch requested a review from kunalspathak September 13, 2022 19:54

kunalspathak reviewed Sep 14, 2022

View reviewed changes

jakobbotsch added 4 commits September 14, 2022 10:54

Leave sections unexpanded by default

17778ad

Address feedback

fa45a99

Revert "Revert forward sub change"

a710c45

This reverts commit e58e404.

Add a horizontal line separator, heading for jit-analyze output

e0cc967

jakobbotsch added 2 commits September 15, 2022 15:18

Address feedback

3fe712a

Merge branch 'main' of github.com:dotnet/runtime into spmi-metrics

d180d77

jakobbotsch added 2 commits September 19, 2022 11:13

Revert "Revert "Revert forward sub change""

1a23870

This reverts commit a710c45.

Random comment change to trigger SPMI without diffs

6910d3c

kunalspathak approved these changes Sep 19, 2022

View reviewed changes

Hide "missed" contexts line when there are none

25285d5

jakobbotsch merged commit c3d09aa into dotnet:main Sep 19, 2022

jakobbotsch deleted the spmi-metrics branch September 19, 2022 17:19

jakobbotsch mentioned this pull request Sep 19, 2022

superpmi-diffs results improvements #75848

Closed

BruceForstall mentioned this pull request Oct 4, 2022

SuperPMI diffs output: split throughput diff by type #73505

Closed

ghost locked as resolved and limited conversation to collaborators Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Enhance metrics reported by superpmi diffs #74584

JIT: Enhance metrics reported by superpmi diffs #74584

jakobbotsch commented Aug 25, 2022

ghost commented Aug 25, 2022

jakobbotsch commented Sep 13, 2022

kunalspathak left a comment

kunalspathak Sep 14, 2022

jakobbotsch Sep 14, 2022

kunalspathak Sep 14, 2022

kunalspathak Sep 14, 2022

jakobbotsch Sep 14, 2022

kunalspathak Sep 14, 2022

jakobbotsch Sep 14, 2022

jakobbotsch Sep 14, 2022 •

edited

Loading

kunalspathak Sep 14, 2022

jakobbotsch commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022 •

edited

Loading

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022 •

edited

Loading

SingleAccretion commented Sep 14, 2022

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 15, 2022

kunalspathak commented Sep 15, 2022

AndyAyersMS commented Sep 16, 2022 •

edited

Loading

jakobbotsch commented Sep 19, 2022

JIT: Enhance metrics reported by superpmi diffs #74584

JIT: Enhance metrics reported by superpmi diffs #74584

Conversation

jakobbotsch commented Aug 25, 2022

ghost commented Aug 25, 2022

jakobbotsch commented Sep 13, 2022

kunalspathak left a 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

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 Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022 • edited Loading

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 14, 2022 • edited Loading

SingleAccretion commented Sep 14, 2022

kunalspathak commented Sep 14, 2022

jakobbotsch commented Sep 15, 2022

kunalspathak commented Sep 15, 2022

AndyAyersMS commented Sep 16, 2022 • edited Loading

jakobbotsch commented Sep 19, 2022

jakobbotsch Sep 14, 2022 •

edited

Loading

jakobbotsch commented Sep 14, 2022 •

edited

Loading

jakobbotsch commented Sep 14, 2022 •

edited

Loading

AndyAyersMS commented Sep 16, 2022 •

edited

Loading