New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 762492 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Regressions for "Tentatively enable FLAG_preparser_scope_analysis"

Project Member Reported by petermarshall@chromium.org, Sep 6 2017

Issue description

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=724cb419ff01c0a913e40c725b9672496441c5f6d14eef70446912176dd132f6


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

android-nexus7v2
chromium-rel-mac-retina
chromium-rel-mac12
chromium-rel-win10
chromium-rel-win7-dual
chromium-rel-win7-gpu-intel
chromium-rel-win7-x64-dual
chromium-rel-win8-dual
Cc: marja@chromium.org
Owner: marja@chromium.org
Status: Assigned (was: Untriaged)

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

Hi marja@chromium.org, 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 : Marja Hölttä
  Commit : 36d703778ccd0d2777e0d4b69ed6e65e39a9f521
  Date   : Mon Sep 04 16:05:39 2017
  Subject: [parser] Tentatively enable FLAG_preparser_scope_analysis.

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : system_health.memory_desktop
  Metric       : memory:chrome:renderer_processes:reported_by_chrome:v8:heap:allocated_objects_size_avg/load_news/load_news_bbc
  Change       : 2.43% | 7118618.66667 -> 7291656.0

Revision                           Result                  N
chromium@499528                    7118619 +- 34674.4      6      good
chromium@499534                    7114439 +- 32119.3      6      good
chromium@499537                    7126683 +- 20834.9      6      good
chromium@499538                    7120059 +- 30123.1      6      good
chromium@499538,v8@1c1457fa70      7121947 +- 34718.5      6      good
chromium@499538,v8@36d703778c      7296917 +- 11926.3      6      bad       <--
chromium@499538,v8@7abdadca0e      7290672 +- 27579.6      6      bad
chromium@499539                    7291656 +- 28360.0      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=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.news.bbc system_health.memory_desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8969198644994061440


For feedback, file a bug with component Speed>Bisection
 Issue 762494  has been merged into this issue.

Comment 5 by marja@chromium.org, Sep 7 2017

This is plausible / expected, since we now put the data related to skipping inner funcs to the heap. Investigating whether this case is hitting some particularly bad behavior.

But yeah, we're trading space for time.

Comment 6 by marja@chromium.org, Sep 7 2017

Investigated this a bit, by just navigating http://www.bbc.co.uk/news/world-asia-china-36189636 (which is roughly what the test does, I guess).

The total amount of scripts is 1.4 MB, and the data generated is 22% of the source code size, which is indeed more than expected. Probably data compacting *is* relevant after all.
Cc: tdresser@google.com
 Issue 763118  has been merged into this issue.
 Issue 763117  has been merged into this issue.
 Issue 763278  has been merged into this issue.
Issue 762498 has been merged into this issue.
Issue 762498 has been merged into this issue.
Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, Sep 10 2017

Issue 762498 has been merged into this issue.
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Sep 11 2017

Cc: jarin@google.com cbruni@chromium.org
 Issue 763773  has been merged into this issue.
Project Member

Comment 14 by 42576172...@developer.gserviceaccount.com, Sep 11 2017

Cc: jarin@chromium.org
 Issue 763771  has been merged into this issue.
Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, Sep 11 2017

 Issue 763771  has been merged into this issue.
Project Member

Comment 16 by 42576172...@developer.gserviceaccount.com, Sep 11 2017

 Issue 763279  has been merged into this issue.
Project Member

Comment 17 by 42576172...@developer.gserviceaccount.com, Sep 11 2017

Cc: jgruber@chromium.org
 Issue 763915  has been merged into this issue.
Cc: hablich@chromium.org
Components: Blink>JavaScript>Parser
Summary: Regressions for "Tentatively enable FLAG_preparser_scope_analysis" (was: 1.1%-2.3% regression in system_health.memory_desktop at 499528:499571)
Experiment: I kicked off some bisects to pinpoint down improvements. 
Project Member

Comment 23 by 42576172...@developer.gserviceaccount.com, Sep 20 2017


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Marja Hölttä
  Commit : 36d703778ccd0d2777e0d4b69ed6e65e39a9f521
  Date   : Mon Sep 04 16:05:39 2017
  Subject: [parser] Tentatively enable FLAG_preparser_scope_analysis.

Bisect Details
  Configuration: mac_retina_perf_bisect
  Benchmark    : loading.desktop
  Metric       : cpuTimeToFirstMeaningfulPaint_avg/pcv1-warm/AirBnB
  Change       : 8.56% | 1147.867 -> 1049.66066667

Revision                           Result                  N
chromium@499528                    1147.87 +- 39.9424      6      good
chromium@499534                    1136.25 +- 18.1436      6      good
chromium@499537                    1143.64 +- 30.5538      6      good
chromium@499538                    1142.46 +- 23.04        6      good
chromium@499538,v8@1c1457fa70      1146.52 +- 26.6393      6      good
chromium@499538,v8@36d703778c      1053.56 +- 28.8326      6      bad       <--
chromium@499538,v8@7abdadca0e      1066.82 +- 57.9556      6      bad
chromium@499539                    1049.66 +- 19.309       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=AirBnB loading.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8967950490354744960


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 24 by 42576172...@developer.gserviceaccount.com, Sep 20 2017


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Marja Hölttä
  Commit : 36d703778ccd0d2777e0d4b69ed6e65e39a9f521
  Date   : Mon Sep 04 16:05:39 2017
  Subject: [parser] Tentatively enable FLAG_preparser_scope_analysis.

Bisect Details
  Configuration: mac_retina_perf_bisect
  Benchmark    : loading.desktop
  Metric       : timeToFirstMeaningfulPaint_avg/pcv1-cold/AirBnB
  Change       : 6.23% | 1307.16111111 -> 1225.71111111

Revision                           Result                  N
chromium@499528                    1307.16 +- 97.167       9       good
chromium@499534                    1306.17 +- 54.6149      9       good
chromium@499537                    1295.16 +- 88.8146      14      good
chromium@499538                    1304.4 +- 64.9538       9       good
chromium@499538,v8@1c1457fa70      1306.47 +- 66.5848      6       good
chromium@499538,v8@36d703778c      1207.35 +- 31.1222      5       bad       <--
chromium@499538,v8@7abdadca0e      1203.05 +- 11.5648      5       bad
chromium@499539                    1225.71 +- 137.066      9       bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=AirBnB loading.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8967950507609633808


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 25 by 42576172...@developer.gserviceaccount.com, Sep 20 2017

📍 Pinpoint job completed.
https://pinpoint-dot-chromeperf.appspot.com/job/149773efb80000

[parser] Tentatively enable FLAG_preparser_scope_analysis.
By marja@chromium.org · Mon Sep 04 16:05:39 2017
v8@36d703778ccd0d2777e0d4b69ed6e65e39a9f521
Project Member

Comment 26 by 42576172...@developer.gserviceaccount.com, Sep 28 2017

 Issue 769693  has been merged into this issue.
Project Member

Comment 27 by bugdroid1@chromium.org, Sep 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a02580636f0529b46940c904f8664df384b2239b

commit a02580636f0529b46940c904f8664df384b2239b
Author: Marja Hölttä <marja@chromium.org>
Date: Fri Sep 29 13:31:13 2017

[parser] Skipping inner funcs: Make the data on heap smaller.

We were unnecessarily storing everything as uint32_t, even though many items in
the preparsed scope data can be stored as uint8_t. This CL also adds an
(internal) API which abstracts away the actual data storing, so the backing
store can be made even more efficient (e.g., use only 1-3 bytes for some
uint32_t values, if they fit) without affecting other parts of the code.

BUG= v8:5516 , chromium:762492 

Change-Id: I7cd4d91dc11f87f8aec9c7584044a6f2a59b73ba
Reviewed-on: https://chromium-review.googlesource.com/684182
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48231}
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/src/factory.cc
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/src/objects/shared-function-info-inl.h
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/src/objects/shared-function-info.h
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/src/parsing/preparsed-scope-data.cc
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/src/parsing/preparsed-scope-data.h
[modify] https://crrev.com/a02580636f0529b46940c904f8664df384b2239b/test/cctest/parsing/test-preparser.cc

Status: Fixed (was: Assigned)
This appears fixed.

Sign in to add a comment