Issue metadata
Sign in to add a comment
|
0.7% regression in sizes at 558805:558806 |
||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 18 2018
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?
,
May 18 2018
,
May 18 2018
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 :)
,
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.
,
May 18 2018
,
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.
,
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.
,
May 22 2018
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 |
|||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, May 18 2018