New issue
Advanced search Search tips

Issue 696086 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

1% regression in memory.top_10_mobile_stress at 451587:451633

Project Member Reported by m...@chromium.org, Feb 24 2017

Issue description

See the link to graphs below.
 

Comment 4 by m...@chromium.org, Feb 25 2017

Restarting bisect...
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Feb 26 2017

Cc: ca...@igalia.com
Owner: ca...@igalia.com

=== 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!
Status: Assigned (was: Untriaged)
This has caused other regressions in memory. Please revert or justify.

Comment 7 by caitp@chromium.org, 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.
Cc: adamk@chromium.org
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.
Cc: gsat...@chromium.org
Labels: Performance-Memory

Comment 11 by m...@chromium.org, Jun 5 2017

Status: WontFix (was: Assigned)
Closing, per recommendation in #c8..
Labels: Performance-Tradeoff

Sign in to add a comment