Issue metadata
Sign in to add a comment
|
11.6% regression in blink_perf.parser at 588253:588276 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Sep 13
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/14632b6b640000
,
Sep 16
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/14632b6b640000 Roll AFDO from 70.0.3538.0_rc-r1 to 71.0.3539.0_rc-r1 by afdo-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com https://chromium.googlesource.com/chromium/src/+/d3977f364f72a1269cb97fa6fb05ebc92b61a52a 819.7 → 723.6 (-96.1) Assigning to sheriff gbiv@chromium.org because "Roll AFDO from 70.0.3538.0_rc-r1 to 71.0.3539.0_rc-r1" is a roll. Understanding performance regressions: http://g.co/ChromePerformanceRegressions Benchmark documentation link: https://bit.ly/blink-perf-benchmarks
,
Sep 18
It's interesting that this range of changes happens, and the benchmark scores instantly become substantially less noisy. There's also no obvious AFDO-induced noise in this benchmark's history, and no recovery in two weeks. I know there was some android-y stabilization effort (by reducing CPU frequency to reduce thermal throttling, etc.) going on a few weeks ago. Will look into it and try to repro locally.
,
Sep 21
+jbroman, mostly for the last paragraph. No such luck; I got a reliable repro on my phone locally. Comparing the `perf` reports for both before and after the roll, `blink::SelectorQuery::FindTraverseRootsAndExecute<blink::AllElementsSelectorQueryTrait>` goes from taking 3% of all cycles to taking 9%. The new profiles never mention this function, whereas the old one has a fair number of samples in it. So, clang considers this function cold on Android. Given that the only user of this function is named `SelectorQuery::QueryAll` and the test is named `query-selector-all-class-deep`, this is likely the source of our regression. :) Note that clang is only treating said function as cold on Android. On Linux, which uses these same profiles to guide optimizations, it'll be optimized as if we used no profile at all. This is because we ask clang to prefer binary size on Android, whereas we prefer speed on Linux. The benchmark history here seems quite quiet since this regression: there's a blip of AFDO helping out after 3539 landed, but the 3555 profile roll -- which, given the perf bugs it gave me, I'm assuming is a flake -- stamped that out. Our general stance is that individual blink microbenchmarks are great indicators of what may be going wrong when a higher-level metric regresses, but they don't provide great signal about how AFDO rolls affect Chrome's performance/snappiness as a whole. I count zero other perf bugs for the 71.0.3539.0_rc-r1 roll, so I'm leaning toward WontFix'ing this. Because a persistent 11% degradation is a lot, I'm tagging a blink_perf.parser owner to be sure that that's an acceptable course of action.
,
Sep 25
Interesting. We have separate fast paths for querySelectorAll for simple tag and class selectors. I guess that wherever our new profiles are coming from, there weren't many uses of qSA for other types of selectors. Assuming we're confident that our new profile is representative of our real use cases, I suppose we can live with this.
,
Sep 25
Closing this out then. Thanks! > Assuming we're confident that our new profile is representative of our real use cases, I suppose we can live with this. At the moment, they're coming from running Chrome on a set of telemetry benchmarks (largely page cyclers, but with some smaller, more targeted benchmarks thrown in, like speeodmeter). We have plans to migrate to field-sourced profiles soonish, which should be highly representative of real use cases :) |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Sep 13