New issue
Advanced search Search tips

Issue 911979 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 889742
Owner:
Closed: Dec 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.7% regression in blink_perf.parser at 611603:611687

Project Member Reported by toyoshim@chromium.org, Dec 5

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=911979

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


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

android-nexus5x-perf

blink_perf.parser - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks
Status: Available (was: Untriaged)
Status: Untriaged (was: Available)
 Issue 911544  has been merged into this issue.
Cc: lfg@chromium.org
Owner: g...@chromium.org
Status: Assigned (was: Untriaged)
The pinpoint job blamed the AFDO roll 5839546069fe6fbf22c2dcd001b45ab55014d2d2.

The graphs recovered on another AFDO roll 96cd045881cdf8b2f652dee05244e43c56e7ae82 and then regressed again after 4dfa4cabde886966917d0b29bf8eab6f53945916.

I know AFDO was a major win is the vast majority of scenarios, but it's quite unfortunate that we see so much variation from one roll to another.

gbiv@, any advice on how to handle these cases?
Issue 889742 sorta tracks that. I was hoping to swing around to that by EOY, but got hit with a pretty nasty bug yesterday that I'm looking into. Sorry that it hasn't bubbled back up to the top of my to-do list (though, on the bright side, this is the first AFDO noise complaint that I've received since before Thanksgiving). For various reasons, after talking with our infra people, it sounds like the resolution of that bug is likely to be "if you have a microbenchmark, please see if it's reasonable to adjust your alerting thresholds to better accommodate AFDO-induced noise."

For this benchmark in particular, at a glance, it looks like Linux, which uses these same AFDO profiles, isn't seeing similar noise. Hence, I suspect that this is a mixture of AFDO and the special "hey clang, please use AFDO profiles to guide size optimizations," flag we pass on Android.

Importantly, this flag will make the compiler way more conservative about inlining in code that the compiler believes is cold. This caused a crash a while back ( issue 851539 ). IIRC, there are some places in this particular benchmark's execution path that need inlining to happen in order to be "fast": https://groups.google.com/a/chromium.org/forum/#!topic/layout-dev/iNikDAR36xk . Namely, PhysicalToLogical<BorderValue>::Before() and others near it. Probably also LogicalToPhysical::* for consistency.

Offhand, I can't say if that's the main or only thing that's causing this much of a swing. But basically, without inlining, code that should only be a few simple instructions instead requires a fair amount of pushes/pops/calls/stack space/etc.

We can try spot-fixing this by adding `__attribute__((always_inline))` to this code and seeing what happens, or by trying to swap to... whatever new API was suggested in the thread above. In a perfect world, this would make Chrome consistently both faster and smaller. In practice... I can't say anything with certainty. :)

In any case, my main priority ATM is issue 916130. Until that and issue 889742 are down, I unfortunately won't be able to help much here. If we can't easily spot-fix this, then I think we'll just have to follow whatever the resolution of issue 889742 is.
Mergedinto: 889742
Status: Duplicate (was: Assigned)
Thanks for the explanation, while I agree that it would be possible to make things more consistent, I'm not sure whether it's worth the effort, which is likely to be pretty significant. I think waiting for a resolution of issue 889742 makes the most sense for now. I'll dupe into that.

Sign in to add a comment