Issue metadata
Sign in to add a comment
|
1.5%-1.7% regression in v8.browsing_mobile_ignition at 441020:441031 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jan 3 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8991446859802131120
,
Jan 4 2017
=== 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!
,
Jan 4 2017
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.
,
Jan 4 2017
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).
,
Jan 6 2017
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.
,
Jan 6 2017
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.
,
Jan 9 2017
,
Apr 18 2017
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 |
|||||||||||||||||||||||
Comment 1 by rmcilroy@chromium.org
, Jan 3 2017