Skip to content

Summary table #1335

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

Closed
wants to merge 2 commits into from
Closed

Summary table #1335

wants to merge 2 commits into from

Conversation

rylev
Copy link
Member

@rylev rylev commented May 24, 2022

This PR replaces the summary section of the comparison page with a summary table mirroring what we show on GitHub and triage reports.

The table is not very nicely formatted, but I wanted to check that this is the direction we want to go before fiddling with CSS.

image

@rylev rylev requested review from Kobzol and Mark-Simulacrum May 24, 2022 10:12
@@ -118,6 +118,7 @@
margin: 0;
padding: 0;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the formatting changes being included in here. Let me know if this is too much a bother, and I can try to remove them.

@@ -222,7 +222,7 @@ impl SiteCtxt {
.map(|bench| {
(
bench.name.as_str().into(),
Category::from_db_representation(&bench.category).unwrap(),
Category::from_db_representation(&bench.category).unwrap_or(Category::Primary),
Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting panics locally without this. Let me know if this should be removed or if we should unwrap into Category::Secondary.

@Kobzol
Copy link
Contributor

Kobzol commented May 24, 2022

Ignoring design for now, I think that the things that we ultimately want to show in the summary is mean and count (maybe max?), for these three axes:

  1. Primary/secondary/all
  2. Regressions/improvements/total (all changes)
  3. All data/filtered data

Would you agree?

So if I'm counting right, there should be 36 or 54 numbers shown. We should probably compress the data somehow. I like the current view, which compresses data well, vs the very large tables.

On top of this, it would be later useful to show data aggregated per profile, per scenario etc., and in that case the summary will probably need to become collapsable (or collapsed by default), otherwise it will simply take too much space.

We could compress the data by merging count and mean into a single "cell". Also I think that it would be ultimately nice to get rid of the All/Filtered distinction. Maybe with some button in the top part that would toggle displaying all results/turning off filters.

# All

# Filtered 
            Primary                  Secondary                     All
     Improvements: 5 (-13 %)     .................        ...................
     Regressions: 2 (2.5 %)      .................        ...................
     Total: 7 (-4 %)             .................        ...................

BTW before making another set of changes to the compare page, maybe it would be worthwhile to resurrect #1213 and split CSS/JS into separate files? The compare page is getting quite complex.

@rylev
Copy link
Member Author

rylev commented May 25, 2022

We could compress the data by merging count and mean into a single "cell".

Where would you put the max figure? And if we want to add more where would they go?

Also I think that it would be ultimately nice to get rid of the All/Filtered distinction

I'm conflicted on this. If the "all" summary is good enough then there's no need for the filter but showing a summary of the filter also allows the user to effectively "query" for this information. One could argue that we should only have a summary of everything on the page and just allow the user to query for whatever subsection they want. I'm not sure that's a good idea though.

maybe it would be worthwhile to resurrect #1213

I think so, but let's do this after this PR so we don't have to resolve conflicts.

@rylev
Copy link
Member Author

rylev commented Aug 15, 2022

Going to close this. I'll probably revisit this in a new PR at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants