Issue metadata
Sign in to add a comment
|
10.7%-11.8% regression in blink_perf.css at 575941:576025 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 23
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15e73843a40000
,
Jul 27
📍 Found significant differences after each of 4 commits. https://pinpoint-dot-chromeperf.appspot.com/job/15e73843a40000 Oilpan: Add Destructor to Promptly Free Stack Allocated HeapVector and HeapDeque by harukamt@google.com https://chromium.googlesource.com/chromium/src/+/6e7487848976f30a8bd3a216e89dafaea48bd979 1.442e+04 → 1.433e+04 (-90.4) [Blink] Avoid crossing editing boundaries selection. by ctzsm@chromium.org https://chromium.googlesource.com/chromium/src/+/7bd29404a6ab8d36bdff4123ae522fcd9068344b 1.433e+04 → 1.422e+04 (-111) [Android] Change suggestions based on frame of focused field by fhorschig@chromium.org https://chromium.googlesource.com/chromium/src/+/4c9cbfb597cd0014ad5200b0a94cf0f13c466af6 1.42e+04 → 1.272e+04 (-1487) gpu: Add disk caching for skia generated shaders in OOP-R. by khushalsagar@chromium.org https://chromium.googlesource.com/chromium/src/+/49b85d7453d643d2d9f7dd127d72c3bdd88ca9c7 1.274e+04 → 1.29e+04 (+166.1) Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Jul 27
fhorschig: The bisect shows a huge drop in the blink_perf.css score for FocusUpdate on Windows 7 at "[Android] Change suggestions based on frame of focused field". That seems really strange as it appears to be an Android-centric change, but can you take a look? The source for the benchmark is https://cs.chromium.org/chromium/src/third_party/blink/perf_tests/css/FocusUpdate.html and you can get more info on the benchmark harness at http://bit.ly/blink-perf
,
Jul 27
Definitely unrelated to my change, the biggest drop in the metric is from: [Android] Change suggestions based on frame of focused field by fhorschig@chromium.org https://chromium.googlesource.com/chromium/src/+/4c9cbfb597cd0014ad5200b0a94cf0f13c466af6 1.42e+04 → 1.272e+04 (-1487) fhorschig@, could this be related to your change?
,
Jul 30
That seems plausible. The intended change is for Android/iOS only, but if I am not wrong, the renderer still emits data on other platforms .... shouldn't be to difficult fixing that.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d79679abaff8f4d5ffa08f2e38f32348b97d3d1e commit d79679abaff8f4d5ffa08f2e38f32348b97d3d1e Author: Friedrich Horschig <fhorschig@chromium.org> Date: Tue Jul 31 11:48:22 2018 Reduce password manager driver requests by caching input field changes Before this CL, the renderer emitted a large amount of redundant messages to the browser: Whenever a focused node changes, there would be a message to the PasswordManagerDriver. With this CL, only relevant requests (when a fillable input field is focused) are always sent to the driver. For multiple focus changes to unfillable fields, only the first change results in a message to the driver. This should fix a performance regression (see linked bug). Running the third_party/blink/perf_tests/css/FocusUpdate.html 20 times resulted in the times below which amount to ~10% difference. Without this CL: median 3359.5858379950205 runs/s ranging from 2484 runs/s to 3464 runs/s With this CL: median 3008 runs/s ranging from 2600 runs/s to 3639 runs/s Bug: 866527 Change-Id: Ief36dea9a7a3f30e83ab9c7454124374b95ed52b Reviewed-on: https://chromium-review.googlesource.com/1154541 Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/master@{#579372} [modify] https://crrev.com/d79679abaff8f4d5ffa08f2e38f32348b97d3d1e/components/autofill/content/renderer/password_autofill_agent.cc [modify] https://crrev.com/d79679abaff8f4d5ffa08f2e38f32348b97d3d1e/components/autofill/content/renderer/password_autofill_agent.h
,
Jul 31
The just merged change showed a reduced median run time on the initially mentioned test (on Linux, though) and I expect it to mitigate this issue. Marking this as fixed. If this solution isn't working as expected, please reopen.
,
Aug 2
This issue existed in 69 already (see issue 865608 which slipped my attention until now). Therefore, requesting a merge back.
,
Aug 2
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 3
Pls apply appropriate OSs label. Thank you.
,
Aug 3
The issue was reported for Windows, but reproducible on Linux. I am fairly certain, that other OSs are affected as well.
,
Aug 3
Is the change looking good in canary and safe to merge to M69? And is it critical change to merge to M69?
,
Aug 6
It is active on Canary and merging this should be quite safe. I am not sure how big the impact of this regression is on 69, so I don't feel strongly about merging this issue. My suggestion is, to have a performance sheriff decide it.
,
Aug 6
,
Aug 6
+futhark, owner of blink_perf.css benchmark: do you feel that this regression fix should be merged?
,
Aug 6
I will wait for futhark@ reply to comment #16. Also Pls note we can take this merge into M69 only if it fully safe and it is truly needed for M69. Thank you.
,
Aug 6
If I understand it correctly, it's about focus changing back and forth in a tight loop of the microbenchmark. It does not sound like it would be an issue in the real world. I'm not familiar with the potential risk of that CL. I'd say it's not necessary to merge.
,
Aug 6
Rejecting merge to M69 based on comment #14 and #18. Thank you. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 23