Skip to content

Switch fraction chart back to absolute values and improve its legend #1806

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

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jan 23, 2024

The fraction chart in benchmark detail on the compare page is designed to show two things:

  1. What is the fraction of frontend/backend/linker within a single artifact collection
  2. How has the absolute time of FE/BE/LNK changed between two artifact collections

However, it is quite challenging (perhaps impossible) to display both of these things in a single chart, without skewing the results.
Before #1775, the chart showed 2), which made 1) skewed. After that PR, the chart shows 1), which doesn't really even allow comparing 2).

Here is an example of the skew that was happening before #1775:
image

In this case, the absolute time of the frontend (purple part) has stayed the same, but its fraction within the collection has changed a lot. But visually, I think that the first thing that my mind when I see it is "the fraction is the same", because the purple rectangle has the same width.

Based on reactions on this PR, it seems that the absolute view is actually OK, and the "skew" isn't problematic for people to undestand. So this PR reverts the display to the absolute value.

Apart from that, the PR also makes tiny stylistic changes in the chart and the legend.

@Kobzol Kobzol requested a review from lqd January 23, 2024 07:54
@nnethercote
Copy link
Contributor

But visually, I think that the first thing that my mind when I see it is "the fraction is the same", because the purple rectangle has the same width.

I don't feel that at all. I strongly prefer the absolute view and find the relative view quite misleading -- with the relative view I immediately think "why did the frontend get slower and the linker faster?" which is quite wrong. You can use the percentages in the text label to get the relative values.

I won't object to having both if you find the relative view useful, but I think absolute should be the default.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 23, 2024

I guess it's quite subjective, yeah. Before, we had the absolute view, and @lqd found it quite confusing and we converged on the relative view 😆

@lqd
Copy link
Member

lqd commented Jan 23, 2024

My recollection is that it wasn’t absolute before, and there were no time values only relative percentages, and that the visualization changed total size sometimes on the before row, sometimes on the after row.

So we were displaying relative percentages of a different total time, making the values incomparable. I also had mentioned a case where I wished to see absolute numbers. So I wasn’t able to compare how much of the process a section took nor the absolute time taken by a section.

As I said before, I believe we probably only need the absolute time each section took (albeit noisy wall times), in a table.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 23, 2024

It's possible that the visualization was different before, I changed the code slightly in the meantime. This PR already added the absolute times to the hover labels, btw.

Copy link
Member

@lqd lqd left a comment

Choose a reason for hiding this comment

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

Maybe the chart legend could also be displaying the expected units, as it's currently showing the relative values even when the chart is in absolute mode.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2024

Maybe the chart legend could also be displaying the expected units

It started to overflow when I included more data 🤦‍♂️ I'll try to tinker with the design a little bit to fix that, the chart probably doesn't need to be so large.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2024

and that the visualization changed total size sometimes on the before row, sometimes on the after row.

This also happens with the new "absolute mode". It needs to work like this, otherwise the width of the longer collection could be unbounded (AFAIK).

@Kobzol Kobzol force-pushed the fraction-chart-ext branch from 76f85ff to cf26f96 Compare January 25, 2024 10:59
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 25, 2024

I did some changes based on the feedback here. Let me know what you think.

chart

@lqd
Copy link
Member

lqd commented Jan 25, 2024

Great!

@Kobzol Kobzol changed the title Allow switching relative and absolute scale of the fraction chart Switch fraction chart back to absolute values and improve its legend Jan 26, 2024
@Kobzol Kobzol merged commit 07a4e12 into rust-lang:master Jan 26, 2024
@Kobzol Kobzol deleted the fraction-chart-ext branch January 26, 2024 15:23
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.

3 participants