Issue metadata
Sign in to add a comment
|
1% regression in memory.top_10_mobile_stress at 451587:451633 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Feb 24 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8986733440858408032
,
Feb 25 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8986649549802250272
,
Feb 25 2017
Restarting bisect...
,
Feb 26 2017
=== Auto-CCing suspected CL author caitp@igalia.com === Hi caitp@igalia.com, 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 : caitp Commit : 0423341034c767d91128c9fd68e3f51e60359c1c Date : Fri Feb 24 17:48:49 2017 Subject: [async-iteration] implement Async-from-Sync Iterator Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : memory.top_10_mobile_stress Metric : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/foreground/http_m_intl_taobao_com_group_purchase_html Change : 1.32% | 3860418.66667 -> 3911314.66667 Revision Result N chromium@452913 3860419 +- 52752.6 6 good chromium@452943 3846041 +- 39309.0 6 good chromium@452958 3860038 +- 53785.1 6 good chromium@452958,v8@0ba513f056 3845811 +- 39648.8 6 good chromium@452958,v8@8cfe45b6f1 3834696 +- 268.149 6 good chromium@452958,v8@0423341034 3925286 +- 50308.6 6 bad <-- chromium@452959 3910635 +- 2691.54 6 bad chromium@452960 3918469 +- 39268.7 6 bad chromium@452962 3918456 +- 39284.1 6 bad chromium@452965 3918175 +- 39629.2 6 bad chromium@452972 3918435 +- 39306.0 6 bad chromium@453030 3911315 +- 255.604 6 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md 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 memory.top_10_mobile_stress Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8986649549802250272 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6219359346753536 | 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!
,
Mar 9 2017
This has caused other regressions in memory. Please revert or justify.
,
Mar 9 2017
There are 2 main things increasing memory use from this CL. 1) v8 snapshot size is up by 60kb. This is caused by some new builtin functions which support the functionality. There are 2 patches open to shrink that snapshot size back down. One is to port the new builtins to C++ (which reduces snapshot size, but increases binary size), fixing a 60kb regression in snapshot size. The other approach refactors the algorithm "InternalPerformPromiseThen" in builtins-promise.cc in v8, replacing 5 Allocate() calls (which produce quite a lot of machine code) with a single one. This shrinks the snapshot by 140kb in x64 release builds, and I believe it is performance neutral, or close to neutral. Both of these patches are up for review, but I've been having trouble getting anyone to look at them this past week. 2) there is a very minor memory increase due to a new Map/hidden class. This is necessary to support the feature, but is unfortunately allocated per-Context rather than per-Isolate. In addition, this becomes a new entry in the "native context" array, growing it by another element. This produces a very slight increase in size for every Context opened. --- Now, I believe the first thing is much more significant than the second one. As I've mentioned, there are fixes submitted for this, but it's been hard to get eyes on them. Both of these regressions are justified as part of implementing a language feature championed and edited by a Googler, which will help get the feature ratified and accepted into the ECMA-262 specification.
,
Mar 10 2017
The regression in snapshot is expected as caitlin explained. Rewriting this in C++ could help reduce the regression but that would result in (a) performance penalty (b) a ton of duplicated code since the async machinery already exists as a codestubassembler interface. There are other low hanging changes in the builtins infrastructure that could help fix the regression. The V8 team plans to look into this next month. I would recommend closing this as a WONTFIX.
,
Mar 10 2017
,
Mar 11 2017
,
Jun 5 2017
Closing, per recommendation in #c8..
,
Jun 21 2017
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by m...@chromium.org
, Feb 24 2017