New issue
Advanced search Search tips

Issue 724076 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

18.2% regression in blink_perf.bindings at 470680:470770

Project Member Reported by tdres...@chromium.org, May 18 2017

Issue description

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

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


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

chromium-rel-mac11-pro
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, May 18 2017

Cc: tapted@chromium.org
Owner: tapted@chromium.org

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

Hi tapted@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 : tapted
  Commit : 335d78a0e0299bda3465cb7e303b79e043f680da
  Date   : Thu May 11 01:19:30 2017
  Subject: Mac[Views]: Make native menus more responsive by pumping private runloop modes.

Bisect Details
  Configuration: mac_pro_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : post-message/post-message
  Change       : 15.94% | 41.8826666667 -> 48.5578333333

Revision             Result                  N
chromium@470679      41.8827 +- 3.36277      6      good
chromium@470725      41.9442 +- 3.13283      6      good
chromium@470748      41.8755 +- 2.24871      6      good
chromium@470759      42.7934 +- 6.49976      9      good
chromium@470765      41.3965 +- 1.76488      6      good
chromium@470768      41.5225 +- 2.67095      6      good
chromium@470769      47.4807 +- 4.37355      6      bad       <--
chromium@470770      48.5578 +- 6.48938      6      bad

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

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

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


| 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!

Comment 4 by tapted@chromium.org, May 18 2017

Cc: yukishiino@chromium.org rsesek@chromium.org haraken@chromium.org bashi@chromium.org
Status: Started (was: Untriaged)
+cc some folks for any tips

(as often seems to be the case with these perf bugs) I'm struggling to find what the test is actually doing, or how to run the test to see what code from that CL is involved.

"blink_perf.bindings" isn't a great key to search for. I think it's related to
https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/Bindings/

that has a bunch of html files with javascript. I'm guessing they are all run? Is there a way to see whether a _particular_ one of these html has blown out its runtime?

It's hard to see how https://codereview.chromium.org/2852233002 plays a role, but perhaps Robert's observation at https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/message_pump_mac.mm#newcode44 is related. i.e. there's an overhead for adding these runloop observers. Perhaps this perf test makes sufficient tasks that it gets noticed. If we can add those observers just to the main UI thread perhaps we succeed in cutting out effects on javascript.

Worth a try implementing the suggestion there I guess. I'll do that.
I'm not an expert on the tests, but some details from the bisect comment:

It posted the command you can run to run the test:
src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings

It listed the metric as post-message/post-message. That's this file in the directory you linked (which is correct): https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/Bindings/post-message.html

Comment 6 by tapted@chromium.org, May 19 2017

Thanks for the pointers! Yeah - I didn't link up that Metric being an actual test case.

CL that potentially fixes this: https://codereview.chromium.org/2889293002

running

$ tools/perf/run_benchmark try mac-pro blink_perf.bindings

("chromium-rel-mac11-pro" wasn't an option, so mac-pro it is)

Comment 7 by tapted@chromium.org, May 19 2017

Seems to be the ticket:

https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/html-results/results-2017-05-18_19-45-51

has post-message going 50.101 ms -> 39.631 ms

Comment 8 by rsesek@chromium.org, May 19 2017

There are definitely costs (in terms of mutexes) to adding CFRunLoop work sources to more modes. The renderers run a NSRunLoop-backed main thread, so if we limit the additional modes to just the NSApplication-backed MessagePump, this should hopefully be fixed.
Project Member

Comment 9 by bugdroid1@chromium.org, May 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80575da0e067c7c941edfd106287dd58ed578f8f

commit 80575da0e067c7c941edfd106287dd58ed578f8f
Author: tapted <tapted@chromium.org>
Date: Sat May 20 00:08:14 2017

Filter run loop modes in message_pump_mac.mm

There's a non-trivial overhead for the machinery to run posted tasks in
a given run loop mode. Only observe modes known to be run for a particular
thread.

BUG=640466,  724076 

Review-Url: https://codereview.chromium.org/2889293002
Cr-Commit-Position: refs/heads/master@{#473391}

[modify] https://crrev.com/80575da0e067c7c941edfd106287dd58ed578f8f/base/message_loop/message_pump_mac.h
[modify] https://crrev.com/80575da0e067c7c941edfd106287dd58ed578f8f/base/message_loop/message_pump_mac.mm

Status: Fixed (was: Started)
Looks recovered
Screen Shot 2017-05-21 at 12.22.59.png
308 KB View Download

Sign in to add a comment