Support passing total code size through when doing SPMI diffs #60124

jakobbotsch · 2021-10-07T13:52:35Z

Currently the total code bytes shown for the base/diff is misleading
when SPMI is used because SPMI generates only .dasm files for method
contexts with diffs. This change:

Adds -baseMetricsSummary and -diffMetricsSummary to SPMI, which
specifies a file path to output metrics. Currently that's just the
number of failing/succeeding compiles, and the total number of code
bytes, but the hope is to use this for other metrics as well.
Adds --override-total-base-metric and --override-total-diff-metric to
jit-analyze, to support overriding the computed total base and total
diff metric, since they are misleading when .dasm files are only
created for differences
Supports this from the superpmi.py wrapper script for asmdiffs when no
explicit metric is provided: in this case, the script will get the
metrics from SPMI and pass them to jit-analyze to override the
computed totals.

The net result is that the displayed results from jit-analyze after SPMI
runs are less misleading and should be more comparable to similar PMI
runs.

Currently the total code bytes shown for the base/diff is misleading when SPMI is used because SPMI generates only .dasm files for method contexts with diffs. This change: * Adds -baseMetricsSummary and -diffMetricsSummary to SPMI, which specifies a file path to output metrics. Currently that's just the number of failing/succeeding compiles, and the total number of code bytes, but the hope is to use this for other metrics as well. * Adds --override-total-base-metric and --override-total-diff-metric to jit-analyze, to support overriding the computed total base and total diff metric, since they are misleading when .dasm files are only created for differences * Supports this from the superpmi.py wrapper script for asmdiffs when no explicit metric is provided: in this case, the script will get the metrics from SPMI and pass them to jit-analyze to override the computed totals. The net result is that the displayed results from jit-analyze after SPMI runs are less misleading and should be more comparable to similar PMI runs.

ghost · 2021-10-07T13:52:45Z

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

Issue Details

Currently the total code bytes shown for the base/diff is misleading
when SPMI is used because SPMI generates only .dasm files for method
contexts with diffs. This change:

Adds -baseMetricsSummary and -diffMetricsSummary to SPMI, which
specifies a file path to output metrics. Currently that's just the
number of failing/succeeding compiles, and the total number of code
bytes, but the hope is to use this for other metrics as well.
Adds --override-total-base-metric and --override-total-diff-metric to
jit-analyze, to support overriding the computed total base and total
diff metric, since they are misleading when .dasm files are only
created for differences
Supports this from the superpmi.py wrapper script for asmdiffs when no
explicit metric is provided: in this case, the script will get the
metrics from SPMI and pass them to jit-analyze to override the
computed totals.

The net result is that the displayed results from jit-analyze after SPMI
runs are less misleading and should be more comparable to similar PMI
runs.

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

jakobbotsch · 2021-10-07T13:53:21Z

cc @dotnet/jit-contrib

BruceForstall · 2021-10-07T16:52:18Z

This fixes #41653 (there is a proposal for generalizing JIT-reported metrics to SPMI, including fixing this, in #52877)

jakobbotsch · 2021-10-07T16:59:55Z

@BruceForstall Indeed, I missed that issue. I don't think the changes to jit-analyze in this PR are what we ultimately will want once we start addressing #52877, but I think for now that it will suffice.

BruceForstall

This looks great; thanks for doing this!

I presume you have a corresponding jit-analyze PR to submit?

jakobbotsch · 2021-10-07T17:21:15Z

I presume you have a corresponding jit-analyze PR to submit?

Whoops, yes, I totally forgot that it won't be part of this PR. Opened dotnet/jitutils#337.

kunalspathak · 2021-10-07T17:29:34Z

Supports this from the superpmi.py wrapper script for asmdiffs when no
explicit metric is provided: in this case, the script will get the
metrics from SPMI and pass them to jit-analyze to override the
computed totals.

May be I am missing something, but will superpmi.py also take some parameters to pass the .csv file? Can you write down a work flow to help me understand?

jakobbotsch · 2021-10-07T18:03:49Z

May be I am missing something, but will superpmi.py also take some parameters to pass the .csv file? Can you write down a work flow to help me understand?

superpmi.py does not have any new args, it passes -baseMetricsSummary and -diffMetricsSummary to superpmi.exe when doing diffs which causes superpmi.exe to write out metrics to those files. superpmi.py then reads those files back and passes the code size metric to jit-analyze.

kunalspathak · 2021-10-07T18:07:42Z

Ok..now I understand and in future, we can make superpmi to dump more things into it.

kunalspathak

LGTM

sandreenko · 2021-11-03T01:23:37Z

How is it supposed to work when "diff" has spmi misses (because of the change) and so it has smaller size, in my case I see:

Total bytes of base: 7178021 (overridden on cmd)
Total bytes of diff: 7171065 (overridden on cmd)
Total bytes of delta: -6956 (-0.10 % of base)
    diff is a regression.
    relative diff is a regression.

Top file regressions (bytes):
           3 : 12751.dasm (2.86% of base)

1 total files with Code Size differences (0 improved, 1 regressed), 1 unchanged.

Top method regressions (bytes):
           3 ( 2.86% of base) : 12751.dasm - System.TimeSpan:Negate():System.TimeSpan:this

Top method regressions (percentages):
           3 ( 2.86% of base) : 12751.dasm - System.TimeSpan:Negate():System.TimeSpan:this

1 total methods with Code Size differences (0 improved, 1 regressed), 1 unchanged.

so it is more misleading than the old version in this scenario, is not it?

BruceForstall · 2021-11-03T01:45:24Z

@sandreenko You're talking about when the "diff" compiler causes SuperPMI to hit "missing data" exceptions in some functions due to changed JIT/EE interface traffic compared to the collection? In that case, we (I believe) fail the asm diff at the superpmi.exe stage, so we don't generate base/diff textual .dasm.

So, yes, it's then biased because anything that failed is, essentially, not considered part of the collection at all. That seems reasonable, though.

Is there some other reason why the overall baseline bytes is different from the overall diff bytes? Did the JIT fail to generate some of the textual .dasm files?

sandreenko · 2021-11-03T02:28:20Z

src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp

@@ -365,11 +368,16 @@ JitInstance::Result JitInstance::CompileMethod(MethodContext* MethodToCompile, i
            pParam->pThis->mc->cr->recAllocGCInfoCapture();

            pParam->pThis->mc->cr->recMessageLog("Successful Compile");
+
+            pParam->metrics->SuccessfulCompiles++;
+            pParam->metrics->NumCodeBytes += NCodeSizeBlock;


@BruceForstall Yes, but if it passes in "base" then we write its size to NumCodeBytes and we don't know that it will fail in diff, so we ignore asm files as you say but this new metric does not look at asm files if I read the change correctly.

So we pass --override-total-base-metric as total number of bytes from compiled methods with base and --override-total-diff-metric as total number of bytes from compiled methods with diff.

I see, we actually are adding the code size before we do the asmdiff. It seems like we should delay until after InvokeNearDiffer is successful (either shows no diffs or some diffs, but doesn't crash) before adding the code size. @jakobbotsch ?

Sure, that might make sense. But if there are misses then posting the summary is misleading in any case because it does not show any diff in those methods, which is probably where the diffs will actually be. So at least this way it is visible to me that there are roughly 7 KB of code that SPMI misses now and that you should probably post PMI diffs instead.

I can change it but I don't see how we can make this case not be misleading to the reader of the summary.

I guess it has to be wordy, like

Total metric of base: 7178021 bytes, 101 methods; Total metric of diff: 7171065 bytes, 100 methods; Total metric of delta: -6956 bytes differs (-0.10% of base), -1 method (-1% of base) Total metric of delta in matching methods: +100 (+0.1 of base) on 100 methods.

But the numbers are not useful, since what I would be interested in as a reader is the diffs in the failed methods. The idea behind this change was to get a better overview of how the change affects libraries crossgen/PMI as a whole and we can never get that when there's missing data. When I see the "Missing data encountered" I always use PMI for my diffs instead.

We could sum the code bytes in the dasm files in jit-analyze and at least give a clear warning that they do not account for all diffs. But this only helps if someone does not notice that SPMI diffs are only partial and then posts the results anyway, not sure if this is what we're afraid of?
Are there are cases where we know there won't be diffs in the functions with missing data encountered so that the SPMI diffs give an accurate view anyway? I can see keeping track of a "number of diffed code bytes" metric as @BruceForstall suggested for this case, but in other cases it seems we should not use SPMI for diffs.

jakobbotsch requested review from BruceForstall and kunalspathak October 7, 2021 13:52

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

jakobbotsch added 3 commits October 7, 2021 17:17

Adapt to use PAL

38b9176

Remove unnecessary makefile change

b049852

Fix GCC build

5577102

jakobbotsch linked an issue Oct 7, 2021 that may be closed by this pull request

superpmi: enable better % improvement number for asm diffs #41653

Closed

BruceForstall approved these changes Oct 7, 2021

View reviewed changes

jakobbotsch mentioned this pull request Oct 7, 2021

Add command line args to override totals shown by jit-analyze dotnet/jitutils#337

Merged

kunalspathak approved these changes Oct 7, 2021

View reviewed changes

jakobbotsch merged commit b66a114 into dotnet:main Oct 8, 2021

jakobbotsch deleted the superpmi-total-asm-size branch October 8, 2021 06:06

sandreenko reviewed Nov 3, 2021

View reviewed changes

sandreenko mentioned this pull request Nov 5, 2021

Set some missing GTF_GLOB_REF and improve its checking. #61245

Closed

ghost locked as resolved and limited conversation to collaborators Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing total code size through when doing SPMI diffs #60124

Support passing total code size through when doing SPMI diffs #60124

jakobbotsch commented Oct 7, 2021

ghost commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

BruceForstall commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

BruceForstall left a comment

jakobbotsch commented Oct 7, 2021

kunalspathak commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

kunalspathak commented Oct 7, 2021

kunalspathak left a comment

sandreenko commented Nov 3, 2021

BruceForstall commented Nov 3, 2021

sandreenko Nov 3, 2021

BruceForstall Nov 3, 2021

jakobbotsch Nov 3, 2021

sandreenko Nov 3, 2021

jakobbotsch Nov 3, 2021

jakobbotsch Nov 3, 2021 •

edited

Loading

Support passing total code size through when doing SPMI diffs #60124

Support passing total code size through when doing SPMI diffs #60124

Conversation

jakobbotsch commented Oct 7, 2021

ghost commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

BruceForstall commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

BruceForstall left a comment

Choose a reason for hiding this comment

jakobbotsch commented Oct 7, 2021

kunalspathak commented Oct 7, 2021

jakobbotsch commented Oct 7, 2021

kunalspathak commented Oct 7, 2021

kunalspathak left a comment

Choose a reason for hiding this comment

sandreenko commented Nov 3, 2021

BruceForstall commented Nov 3, 2021

sandreenko Nov 3, 2021

Choose a reason for hiding this comment

BruceForstall Nov 3, 2021

Choose a reason for hiding this comment

jakobbotsch Nov 3, 2021

Choose a reason for hiding this comment

sandreenko Nov 3, 2021

Choose a reason for hiding this comment

jakobbotsch Nov 3, 2021

Choose a reason for hiding this comment

jakobbotsch Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

jakobbotsch Nov 3, 2021 •

edited

Loading