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

Issue 719530 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

12.3% regression in blink_perf.parser at 469616:469654

Project Member Reported by kraynov@chromium.org, May 8 2017

Issue description

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

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


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

android-nexus5
Cc: gab@chromium.org
Owner: gab@chromium.org

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

Hi gab@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 : gab
  Commit : 7af9dc076d2349ade19a95e24f6ee6811e939765
  Date   : Fri May 05 13:38:54 2017
  Subject: Make nesting/running states a RunLoop rather than a MessageLoop concept.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : blink_perf.parser
  Metric       : textarea-parsing/textarea-parsing
  Change       : 12.63% | 140.057941767 -> 122.370515435

Revision             Result                   N
chromium@469615      140.058 +- 1.53271       6      good
chromium@469635      139.256 +- 1.83633       6      good
chromium@469636      121.583 +- 2.56063       6      bad       <--
chromium@469637      122.592 +- 2.30603       6      bad
chromium@469638      122.214 +- 1.19008       6      bad
chromium@469640      122.254 +- 2.51235       6      bad
chromium@469645      122.406 +- 1.43699       6      bad
chromium@469654      122.371 +- 0.418935      6      bad

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

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

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


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

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

Suspected Commit
  Author : gab
  Commit : 7af9dc076d2349ade19a95e24f6ee6811e939765
  Date   : Fri May 05 13:38:54 2017
  Subject: Make nesting/running states a RunLoop rather than a MessageLoop concept.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : blink_perf.parser
  Metric       : textarea-parsing/textarea-parsing
  Change       : 12.70% | 140.368400688 -> 122.545366133

Revision             Result                  N
chromium@469615      140.368 +- 1.82076      6      good
chromium@469635      139.93 +- 1.79148       6      good
chromium@469636      121.578 +- 2.93752      6      bad       <--
chromium@469637      121.918 +- 2.10799      6      bad
chromium@469638      121.65 +- 1.47114       6      bad
chromium@469640      121.303 +- 0.83739      6      bad
chromium@469645      121.461 +- 1.95554      5      bad
chromium@469654      122.545 +- 2.47184      6      bad

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

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

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


| 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 6 by gab@chromium.org, May 10 2017

Labels: ReleaseBlock-Beta
Status: Assigned (was: Untriaged)
I have a theory that this is the change to third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.cc in https://codereview.chromium.org/2818533003 which replaces a member method call with a mandatory TLS call.

And since IsNested() is called in the Blink scheduler's work loop [1] this could have a performance impact?

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc?rcl=41d0368bc22967ae30b2b5097d887f6552efff83&l=286

I have an idea on how to solve it through the step that always going to follow this CL (RunLoop::Delegate), so I'll go ahead and implement that instead of reverting this CL.

Comment 7 by gab@chromium.org, May 10 2017

Cc: skyos...@chromium.org
@skyostil: does #6 make sense to you?
Please tag with applicable OSs.  Thanks.

Comment 9 by gab@chromium.org, May 11 2017

Labels: OS-Android
Hmm, TLS should be ~free on Android so I'm not sure what's going on here. Is it possible we're falsely thinking that we're nested when we're not? The scheduler turns off most optimizations when it's nested.
Project Member

Comment 11 by bugdroid1@chromium.org, May 18 2017

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

commit 27355196d32f75606b3e43b54bd0d03ef42b4579
Author: gab <gab@chromium.org>
Date: Thu May 18 06:01:10 2017

Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more.

Separates RunLoop/MessageLoop further. Still some interdependencies to address
but this drops friend class RunLoop; from MessageLoop :).

It also moves the relevant state from TLS back onto the MessageLoop instance
(hidden as private data in RunLoop::Delegate). This is easier to manage
and ensures cleanup when MessageLoop goes away (particularly important in unit
tests).

BUG= 703346 ,  719530 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

TBR=phajdan.jr@chromium.org for content\public\test\cache_test_util.h fwd-decl nit

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

[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/message_loop/message_loop.cc
[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/message_loop/message_loop.h
[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/run_loop.cc
[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/run_loop.h
[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/components/download/internal/scheduler/network_listener_unittest.cc
[modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/content/public/test/cache_test_util.h

Comment 12 by gab@chromium.org, May 19 2017

Status: Fixed (was: Assigned)
Fixed by r472695 (IsNested() is still done from TLS directly so that wasn't it).

My theory is that this regression was mostly a hoax, maybe because the test is constructing/destroying MessageLoops in a loop or something (I think previous destruction handling was a bit heavier -- the state now lives privately inside MessageLoop instead of in TLS).

Oh well... new design is better for other reasons anyways :)

Sign in to add a comment