New issue
Advanced search Search tips

Issue 844622 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

0.7% regression in sizes at 558805:558806

Project Member Reported by npm@chromium.org, May 18 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, May 18 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=844622

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=4fa1ccd41d209495e3006414fb0cbb9383fe3e84ef9911c691f38096333a6079


Bot(s) for this bug's original alert(s):

linux

Comment 2 by npm@chromium.org, May 18 2018

Owner: g...@chromium.org
Bisect does not work for this one. Looking at the position where the graph goes, it looks like the cause is b4a5f27eb04fc91e37c491ee8680ad0e37feb5d6

gbiv@, could you take a look?

Comment 3 by npm@chromium.org, May 18 2018

Status: Assigned (was: Untriaged)

Comment 4 by g...@chromium.org, May 18 2018

Status: WontFix (was: Assigned)
Hm, I was originally expecting more around a 0.4% binary size regression for this change, but I think 0.7% is reasonable, given that it's meant to improve Chrome's overall performance by 4-7% percent.

Given that it's been relatively stable afterward, this looks fine to me.

Thanks for the heads up :)

Comment 5 by npm@chromium.org, May 18 2018

Hm Ok, I see this was approved by thakis@ so I imagine there was appropriate discussion. I'm not sure what you mean by 'improve Chrome's overall performance by 4-7% percent' (there are many metrics, not one). It would be nice to have included precise explanation of expected performance gains in the CL description.

Comment 6 by npm@chromium.org, May 18 2018

Cc: thakis@chromium.org

Comment 7 by g...@chromium.org, May 18 2018

> I'm not sure what you mean by 'improve Chrome's overall performance by 4-7% percent' (there are many metrics, not one). It would be nice to have included precise explanation of expected performance gains in the CL description

Yeah, I've yet to figure out a good way to communicate AFDO's improvements in a sentence or two. The long story short is that this change feeds samples of how Chrome runs into the compiler, so the can make better optimization decisions (what to inline, what to vectorize, how to lay code out, ...) throughout Chromium's codebase. So, in general, CPU-intensive metrics should benefit from this change. I did a fair amount of benchmarking here: https://bugs.chromium.org/p/chromium/issues/detail?id=821595 and have an (Google-only, sorry) sheet summarizing all of those reports in pretty graphs.

Some individual metrics were sped up by 2%, some were sped up by 5%, some 10%, others were in the noise, ... but noise and the sheer number of metrics taken makes it sort of impractical to precisely say "here's exactly what we expect". If you (or anyone) have suggestions for how to succinctly communicate this, I'm happy to hear them. :)

I really should've pointed people toward what AFDO is/summaries of it in the description, though. Sorry about that.

Comment 8 by thakis@chromium.org, May 19 2018

0.7% is pretty large. Let's wait until this has reached stable and see how perf changes in the wild then though, and then reconsider what the concrete numbers we're trading off are.

Comment 9 by g...@chromium.org, May 22 2018

Status: Assigned (was: WontFix)
WFM.

Sounds like I incorrectly assumed that we're not highly sensitive to binary size increases. In that case, I'll reopen this until we decide whether or not the 0.7% is worth it.

Additionally, there's a flag we can pass to make AFDO prefer to save binary size over granting performance. Applying it looks like it drops a 64-bit `chrome.stripped` from 131,860KB to 113,732KB. Without the flag (and with AFDO manually turned off), it's 131,192KB. My baseline numbers are admittedly a bit lower than what this chart shows, but I'd imagine that the binary size difference will be similar.

In my Android investigation, it made turning AFDO on a perf loss for a nontrivial number of benchmarks, though in aggregate, we improved more numbers than we regressed. I don't know how well this transfers to Linux, given that Android was already built with -Oz everywhere.

Assuming that saving $N MB of binary size may be preferable to saving $M CPU cycles, I'll gather some numbers for what the perf hit of this might look like while we're waiting for stable to pick up AFDO. I'd try to cite a benchmark or two now, but pinpoint is giving me 500s and not running my anything at the moment, sooo. :)

Sign in to add a comment