New issue
Advanced search Search tips

Issue 866527 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 31
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.7%-11.8% regression in blink_perf.css at 575941:576025

Project Member Reported by sullivan@chromium.org, Jul 23

Issue description

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

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


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

Win 7 Nvidia GPU Perf
Win 7 Perf
Cc: fhorschig@chromium.org harukamt@google.com ctzsm@chromium.org khushals...@chromium.org
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)
📍 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
Owner: fhorschig@chromium.org
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
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?
Status: Started (was: Assigned)
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.
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.
Labels: Merge-Request-69
This issue existed in 69 already (see issue 865608 which slipped my attention until now).
Therefore, requesting a merge back.
Project Member

Comment 10 by sheriffbot@chromium.org, Aug 2

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
Pls apply appropriate OSs label. Thank you.
Labels: OS-Linux OS-Windows
The issue was reported for Windows, but reproducible on Linux.

I am fairly certain, that other OSs are affected as well.
Is the change looking good in canary and safe to merge to M69? And is it critical change to merge to M69? 
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.
Cc: -harukamt@google.com
Cc: futhark@chromium.org
+futhark, owner of blink_perf.css benchmark: do you feel that this regression fix should be merged?
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.
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.

Labels: -Merge-Review-69 Merge-Rejected-69
Rejecting merge to M69 based on comment #14 and #18. Thank you.

Sign in to add a comment