New issue
Advanced search Search tips

Issue 637355 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.1% regression in blink_perf.bindings at 410807:411148

Project Member Reported by benjhayden@chromium.org, Aug 12 2016

Issue description

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

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


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

android-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Aug 12 2016

Cc: esprehn@chromium.org
Owner: esprehn@chromium.org

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

Hi esprehn@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Use StringView for String::reverseFind.
Author  : esprehn
Commit description:
  
This avoids the allocations for any callers passing literal strings.

I also made AtomicString and String's API match.

BUG= 615174 

Review-Url: https://codereview.chromium.org/2226363003
Cr-Commit-Position: refs/heads/master@{#410930}
Commit  : 8d2cf7c3e866ff9ec49a8b5d544dde87f10716c4
Date    : Wed Aug 10 02:47:41 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@410806  182.558  4.50875  12  good
chromium@410892  182.15   1.79072  8   good
chromium@410914  186.659  4.36375  8   good
chromium@410925  186.198  4.68518  8   good
chromium@410928  188.79   3.65427  8   good
chromium@410929  187.585  4.19346  8   good
chromium@410930  194.536  4.26673  5   bad    <--
chromium@410935  193.365  3.44945  8   bad
chromium@410977  194.019  3.29274  5   bad
chromium@411148  188.699  6.0856   12  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 637355

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.bindings
Test Metric: serialize-nested-array/serialize-nested-array
Relative Change: 6.20%
Score: 95.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2461
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004512290036242528


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6465737699360768

| 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 Tests>AutoBisect.  Thank you!
Cc: jbroman@chromium.org
Labels: Needs-Bisect
Owner: ----
Status: Available (was: Assigned)
This benchmark is serializing a 10k deep nested array with postMessage:

https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/Bindings/serialize-nested-array.html?q=serialize-nested-array&sq=package:chromium&l=1

I don't see reverseFind anywhere in the code that runs for that. Maybe a bad bisect?

jbroman@ Any ideas?
Components: Blink>Bindings
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Aug 13 2016

Cc: fdoray@chromium.org
Owner: fdoray@chromium.org

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

Hi fdoray@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Make ThreadChecker::CalledOnValidThread() return true when called from the same task.
Author  : fdoray
Commit description:
  
This CL introduces TaskToken to identify individual tasks. In the scope
of a ScopedSetSequenceTokenForCurrentThread, a unique TaskToken is set
in TLS. This unique TaskToken can be retrieved using
TaskToken::GetForCurrentThread().

ThreadCheckerImpl uses TaskToken to make CalledOnValidThread() return
true when called multiple times from the same task. This allows usage
of ThreadChecker/NonThreadSafe objects from the stack on tasks not
otherwise running in a single-threaded context.

BUG= 553459 

Review-Url: https://codereview.chromium.org/2213263002
Cr-Commit-Position: refs/heads/master@{#410839}
Commit  : b339954beda5b00c4a482d74594cbe2e7cd39516
Date    : Tue Aug 09 21:51:59 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@410500  183.732  2.71002   8  good
chromium@410700  179.01   2.13525   5  good
chromium@410800  175.631  0.974107  5  good
chromium@410825  172.381  3.2724    5  good
chromium@410838  181.414  2.89706   5  good
chromium@410839  194.118  2.4172    5  bad    <--
chromium@410840  196.904  1.38411   5  bad
chromium@410841  194.207  2.31019   5  bad
chromium@410844  192.376  3.03331   5  bad
chromium@410850  194.234  3.4627    5  bad
chromium@410900  189.197  3.44585   8  bad
chromium@411300  193.507  1.45272   8  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 637355

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.bindings
Test Metric: serialize-nested-array/serialize-nested-array
Relative Change: 5.15%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2462
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004479341002485472


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5281912642338816

| 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 Tests>AutoBisect.  Thank you!
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Aug 13 2016

Cc: puthik@chromium.org
Owner: puthik@chromium.org

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

Hi puthik@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : arc: bluetooth: Add/Remove BT observer only when ARC is ready
Author  : puthik
Commit description:
  
The current implementation make arc_bluetooth_bridge to be
a bluetooth observer all the time. This lead to unnessary
work when ARC is not ready.

This patch changes that by register arc_bluetooth_bridge
to be a bluetooth observer when arc is ready and unregister
when arc is gone

BUG=635578
TEST=No log spam when there is no arc support

Review-Url: https://codereview.chromium.org/2223203002
Cr-Commit-Position: refs/heads/master@{#410828}
Commit  : 1ffc3026c8758d9464e76d8ea0fd0c542906e4cb
Date    : Tue Aug 09 21:31:13 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@410500  184.608  2.4269    5  good
chromium@410700  185.373  2.08948   5  good
chromium@410800  179.908  3.20692   5  good
chromium@410825  173.609  2.74746   5  good
chromium@410827  175.018  0.507225  5  good
chromium@410828  186.094  2.89261   5  bad    <--
chromium@410829  185.674  2.44598   5  bad
chromium@410832  182.835  4.70972   5  bad
chromium@410838  184.163  5.40581   5  bad
chromium@410850  196.767  3.86685   5  bad
chromium@410900  192.859  4.20193   5  bad
chromium@411300  193.113  2.291     5  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 637355

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.bindings
Test Metric: serialize-nested-array/serialize-nested-array
Relative Change: 4.61%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2463
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004441813092922480


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5896263204077568

| 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 Tests>AutoBisect.  Thank you!
Owner: fdoray@chromium.org
I disagree with the bot's analysis of the last run. It looks like the regression is the one from ~180 to ~195, which the last run also places between 410838 and 410850:

https://chromium.googlesource.com/chromium/src/+log/385af3c5b3cac10f492b0cb657714a7fd3ad7cd6..7c4caf18ed934d397524c12f52978af01cfa70ec

In that range, fdoray's CL that the second run homed in on does seem like the most likely culprit.
I assume that performance tests run on a build without DCHECKs? (correct me if I'm wrong)

My CL https://codereview.chromium.org/2213263002 modified ThreadChecker which is a no-op on non-DCHECK builds https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?sq=package:chromium&dr=CSs&l=64 . It is therefore unlikely to have caused this regression. 
Owner: ----
Could someone reply to comment #11?
Yes, DCHECK builds are very slow. :)
Cc: benjhayden@chromium.org
Status: WontFix (was: Available)
Alright, given what fdoray@ said, it sounds like that CL should not have affected performance on non-DCHECK builds, so it sounds like we still haven't found a convincing cause.

To me, it looks like https://chromeperf.appspot.com/group_report?bug_id=637355 is just noise; I think it's not a real regression.

Please re-open if you think further investigation is warranted :-)

Sign in to add a comment