Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPMI: Use diffed bytes for jit-analyze and print more info #61254

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Nov 5, 2021

Fix the misleading total bytes displayed when diffing with SPMI. If one
of the JITs encountered missing data, the code bytes would not be
included, while the other JIT could still have it included if it did not
encounter missing data.

Also add more information about missing SPMI data/successful replays
printed. For replays that is useful to be able to gauge whether there is
still ok coverage after a large change. For diffs, we print a warning
for missing SPMI data that the diff summary may be misleading.

Example for a successful replay:
Clean SuperPMI replay (219868 contexts processed)

Example for a replay with missing data:
SuperPMI encountered missing data for 6 out of 27272 contexts

Example warning printed:
Warning: SuperPMI encountered missing data during the diff. The diff summary printed above may be misleading.
Missing with base JIT: 0. Missing with diff JIT: 6. Total contexts: 27272.

Fix the misleading total bytes displayed when diffing with SPMI. If one
of the JITs encountered missing data, the code bytes would not be
included, while the other JIT could still have it included if it did not
encounter missing data.

Also add more information about missing SPMI data/successful replays
printed. For replays that is useful to be able to gauge whether there is
still ok coverage after a large change. For diffs, we print a warning
for missing SPMI data that the diff summary may be misleading.

Example for a successful replay:
    Clean SuperPMI replay (219868 contexts processed)

Example for a replay with missing data:
    SuperPMI encountered missing data for 6 out of 27272 contexts

Example warning printed:
    Warning: SuperPMI encountered missing data during the diff. The diff
    summary printed above may be misleading.
    Missing with base JIT: 0. Missing with diff JIT: 6. Total contexts:
    27272.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 5, 2021
@ghost
Copy link

ghost commented Nov 5, 2021

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

Issue Details

Fix the misleading total bytes displayed when diffing with SPMI. If one
of the JITs encountered missing data, the code bytes would not be
included, while the other JIT could still have it included if it did not
encounter missing data.

Also add more information about missing SPMI data/successful replays
printed. For replays that is useful to be able to gauge whether there is
still ok coverage after a large change. For diffs, we print a warning
for missing SPMI data that the diff summary may be misleading.

Example for a successful replay:
Clean SuperPMI replay (219868 contexts processed)

Example for a replay with missing data:
SuperPMI encountered missing data for 6 out of 27272 contexts

Example warning printed:
Warning: SuperPMI encountered missing data during the diff. The diff
summary printed above may be misleading.
Missing with base JIT: 0. Missing with diff JIT: 6. Total contexts:
27272.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Nov 5, 2021

@sandreenko for your PR this prints the following to console now:

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 7170863 (overridden on cmd)
Total bytes of diff: 7170866 (overridden on cmd)
Total bytes of delta: 3 (0.00 % of base)
    diff is a regression.
    relative diff is a regression.


Top file regressions (bytes):
           3 : 11295.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) : 11295.dasm - System.TimeSpan:Negate():System.TimeSpan:this

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

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

--------------------------------------------------------------------------------
Warning: SuperPMI encountered missing data during the diff. The diff summary printed above may be misleading.
Missing with base JIT: 0. Missing with diff JIT: 6. Total contexts: 27272.

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jakobbotsch jakobbotsch merged commit 9c37cdc into dotnet:main Nov 9, 2021
@jakobbotsch jakobbotsch deleted the better-diff-metrics branch November 9, 2021 18:23
@BruceForstall
Copy link
Member

@jakobbotsch It looks like superpmi.py will use the new "override base/diff metric" logic if the --metrics option is not used, so we end up using the default CodeSize metric. What if I use --metrics CodeSize,PerfScore? In that case, we could override just the CodeSize metrics, given a more general --override-total-base-metric argument to jit-analyze that takes <metric>,<value> pairs (say). I guess we don't have the PerfScore override until something like #52877 is implemented.

@jakobbotsch
Copy link
Member Author

I agree that sounds like a fine way to do it. FWIW, my envisioning was to eventually pass the base and diff summary metrics files themselves (instead of just the numbers of the code size metric) to jit-analyze and have jit-analyze use directly numbers from there to present other metrics as well (e.g. perf score once we have a good way of getting it (#52877), memory allocations).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants