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

superpmi-diffs results improvements #75848

Closed
jakobbotsch opened this issue Sep 19, 2022 · 8 comments
Closed

superpmi-diffs results improvements #75848

jakobbotsch opened this issue Sep 19, 2022 · 8 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 19, 2022

Some suggested follow-ups from #74584

category:eng-sys
theme:super-pmi
skill-level:beginner
cost:small
impact:small

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jakobbotsch jakobbotsch self-assigned this Sep 19, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 19, 2022
@jakobbotsch jakobbotsch added this to the 8.0.0 milestone Sep 19, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 19, 2022
@jakobbotsch jakobbotsch added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Sep 19, 2022
@ghost
Copy link

ghost commented Sep 19, 2022

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

Issue Details

Some suggested follow-ups from #74584

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr, untriaged

Milestone: 8.0.0

@EgorBo
Copy link
Member

EgorBo commented Sep 19, 2022

Can we display a couple of real diffs on the summary page? e.g. for a few smallest methods

@jakobbotsch
Copy link
Member Author

Can we display a couple of real diffs on the summary page? e.g. for a few smallest methods

Would be nice too.

Also if we see any TP diffs in FullOpts we currently display the rows for those collections in all tables; we should consider not showing the rows in the MinOpts table if there are no diffs (for asm diffs it's harder, as we cannot tell if there are diffs or not just based on "0 byte" diffs).

jakobbotsch added a commit that referenced this issue Oct 6, 2022
Write out a breakdown of improvements/regressions to be able to gauge better how the improvements stack up against the regressions.

Contributes to #75848
@BruceForstall
Copy link
Member

If there are no diffs, I'd like to see that displayed prominently at the top. Currently, you need to visually examine each platform to notice that. Also, I think "No diffs" (or maybe the corresponding "X diffs"?) should be slightly larger font and first, per-section.

It seems like "Diffs are based on ..." can be in the "Detail" expansion. I understand if we want to keep "MISSING" in the main view.

e.g.,:

ASM diffs generated on Windows x64
No diffs found on any platform or Diffs found on 3 of 5 platforms

Linux arm64
No diffs found.
MISSED contexts: base: 846, diff: 846
Details
...

@jakobbotsch
Copy link
Member Author

Sorry @BruceForstall, I have forgotten to respond to that.

If there are no diffs, I'd like to see that displayed prominently at the top. Currently, you need to visually examine each platform to notice that.

I agree something like that would be nice, but currently we do not have a model of the results of the other runs. The overall diffs summary is just a concatenation of the summaries created by child SPMI runs. I suppose we could check for "No diffs found" or something similar in those markdown files.

It seems like "Diffs are based on ..." can be in the "Detail" expansion. I understand if we want to keep "MISSING" in the main view.

The original intent was to make it clear that there is a reasonable set of methods in the collection to make conclusions based on the results, but I agree that it could be moved under the details section.

@BruceForstall
Copy link
Member

but currently we do not have a model of the results of the other runs.

I previously opened #80719, which is related, suggesting some alternatives to how summarization is implemented. E.g., have the individual jobs create a data model of their results and a "Summarization" job combine them into a report. Would probably be a fair amount of work, with possibly not a huge amount of benefit.

@jakobbotsch
Copy link
Member Author

Most of this was addressed, so will close this. I think we can consider changing summarization once we address #80719.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 6, 2023
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

No branches or pull requests

3 participants