New issue
Advanced search Search tips

Issue 678101 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

1.5%-1.7% regression in v8.browsing_mobile_ignition at 441020:441031

Project Member Reported by rmcilroy@chromium.org, Jan 3 2017

Issue description

See the link to graphs below.
 
Cc: gsat...@chromium.org
Owner: gsat...@chromium.org

=== PERF REGRESSION ===


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

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


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


===== SUSPECTED CL(s) =====
Subject : [promises] Remove deferred object
Author  : gsathya
Commit description:
  
This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

This patch results in a 8.5% improvement in benchmarks over 5 runs.

BUG= v8:5343 

Review-Url: https://codereview.chromium.org/2590563003
Cr-Commit-Position: refs/heads/master@{#41991}
Commit  : 5668ce39873f97f0bd8b25289f41c88cf91aa9bc
Date    : Thu Dec 29 20:30:28 2016


===== TESTED REVISIONS =====
Revision                       Mean     Std Dev  N  Good?
chromium@441023                2355169  25932.8  6  good
chromium@441023,v8@f2e8c9786f  2357822  30098.9  6  good
chromium@441023,v8@26c293a93b  2367704  25677.4  6  good
chromium@441023,v8@5668ce3987  2399491  24457.8  6  bad    <--
chromium@441023,v8@7ad54344c1  2398715  26553.8  6  bad
chromium@441024                2391420  22020.4  6  bad

Bisect job ran on: android_nexus5_perf_bisect
Bug ID: 678101

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.news.washingtonpost v8.browsing_mobile_ignition
Test Metric: memory:chrome:renderer_processes:reported_by_chrome:v8:code_and_metadata_size_max/browse_news/browse_news_washingtonpost
Relative Change: 1.54%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/4496
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8991446859802131120


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

| 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!
I think this might be because the slow path (when we create more than one callback) got slower since we allocate three FixedArrays, each of size 2. Before this CL, we used to allocate only two. Wapo being the only affected site seems to indicate that they probably hit this slow path a lot (as opposed something inherently slower in Promises resulting from this CL).

Ross, can you please point me to the benchmark code? I can't find it in the v8-perf repo. I will try to reproduce this.
Thanks for looking into this Sathya. This is measuring memory usage (specifically memory of JITed code) not time. The benchmark is a Chrome benchmark, so it's not part of v8-perf, so you will need to run the test from the Chromium repo (and possibly only repos on Android, so you might need to build and run on an Android device. 

The test command is in the bisect output: 
src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.news.washingtonpost v8.browsing_mobile_ignition

(note the -browser=android-chromium which will try to run on an Android device over ADB, you can change that if you want to test on desktop first).
Oh, right, sorry. It does make sense that there is more memory being used since we allocate more FixedArrays.

Looking at comment3, the standard deviation numbers for f2e8c9786f makes me suspicious if this CL is even the cause for the regression, though. 
I don't think stddev is calculated correctly on these benchmarks (I think it incorrectly takes the stddev across all pages in the benchmark, not the stddev across repeat runs of the same page). Either way, from the mean values it seems pretty clear the regression was on that CL on the bisect in both bugs.

Also, this is code memory (i.e., JITed code, TF stubs and bytecode), not the heap memory, so the regression is not due to allocation of the FixedArrays.
Status: Assigned (was: Untriaged)
Status: WontFix (was: Assigned)
Closing as WONTFIX as this is because of moving builtins to CSA. See https://bugs.chromium.org/p/chromium/issues/detail?id=695852#c14 for more information.

Sign in to add a comment