New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727626 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

20.2% regression in blink_perf.bindings at 474890:474904

Project Member Reported by toyoshim@chromium.org, May 30 2017

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg-vyN8QgM


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

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 13 2017

Cc: bmcquade@chromium.org
Owner: bmcquade@chromium.org

=== Auto-CCing suspected CL author bmcquade@chromium.org ===

Hi bmcquade@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : bmcquade
  Commit : 42b72063edb66bfd015bd9dea116c3e3e937d3eb
  Date   : Fri May 26 03:14:11 2017
  Subject: Provide WebContents::CreateParams to tab helpers.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : get-elements-by-tag-name/get-elements-by-tag-name
  Change       : 22.59% | 133.078095065 -> 103.014572285

Revision             Result                  N
chromium@474000      133.078 +- 5.08569      6      good
chromium@474452      135.857 +- 5.73003      6      good
chromium@474678      130.101 +- 4.53407      5      good
chromium@474791      142.163 +- 4.65086      6      good
chromium@474848      132.121 +- 5.40164      6      good
chromium@474876      126.186 +- 3.26932      6      good
chromium@474890      128.163 +- 3.25432      6      good
chromium@474894      127.676 +- 3.56766      6      good
chromium@474895      106.394 +- 4.99329      6      bad       <--
chromium@474896      105.304 +- 8.77445      6      bad
chromium@474897      105.302 +- 6.33683      6      bad
chromium@474904      103.015 +- 3.32356      6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8976996886640299520

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6504581887950848


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!
This seems like a bad bisect. This change simply passes a new parameter to a few methods, and does nothing interesting / complex with that param.
Owner: ----
Looking at the test a bit more, it just calls getElementsByTagName in a loop. My change only affects things when a WebContents is created, and even then, its effect is minimal, so I'm very skeptical that this bisect is correct. I'd like to re-run the bisect to double check, but I can't figure out where in the perf dash UI I can do this. :-( Maybe the UI could provide a button for this, as it seems like a very common action users may want to take.

In the meantime I'm unassigning this from myself.
Unassigning with an explanation is the right thing to do, thanks! I reran the bisect. For future reference, you can click the exclamation point in the graph and click the "Bisect" button in the tooltip that comes up to re-run.
Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, Jun 17 2017


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : bmcquade
  Commit : 42b72063edb66bfd015bd9dea116c3e3e937d3eb
  Date   : Fri May 26 03:14:11 2017
  Subject: Provide WebContents::CreateParams to tab helpers.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : get-elements-by-tag-name/get-elements-by-tag-name
  Change       : 19.40% | 127.190133976 -> 102.510479811

Revision             Result                  N
chromium@474889      127.19 +- 1.03951       6      good
chromium@474893      127.795 +- 2.31098      6      good
chromium@474894      127.735 +- 2.38563      6      good
chromium@474895      105.92 +- 5.73755       6      bad       <--
chromium@474897      103.977 +- 5.11399      6      bad
chromium@474904      102.51 +- 1.08246       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8976671876171177120

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6232906046898176


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!
Owner: bmcquade@chromium.org
This is quite puzzling - looks like it picked out my change again.

I can try to repro this locally next week with and without the patch applied, to see if it repros. If so, I'll debug to see if I can figure out how my change could cause this. Still quite skeptical.
Cc: jbroman@chromium.org
+jbroman, owner of blink_perf.bindings benchmark, in case he has any ideas for how bmcquade should investigate.
jbroman, if you're willing to take a scan of the blamed change and see if you can see how this might cause this regression, I'd really appreciate it. The 2 look totally unrelated to me, but it's possible I'm missing something.

https://codereview.chromium.org/2894973002

Thanks!
Status: Assigned (was: Untriaged)
Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md

We're looking for one of the following:
1. Justification via explanation
2. Plan to revert or fix
3. Angry rage throwing of equipment at my head

Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it.

Note: This was a bulk edit message and not very personal.
Status: WontFix (was: Assigned)
This change was reverted for other reasons. We think the bisect result is probably wrong, but in any case, the change was reverted, so I'm closing this out.

Sign in to add a comment