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

Issue 732482 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

10%-11.1% regression in media.tough_video_cases_tbmv2 at 478302:478441

Project Member Reported by chcunningham@chromium.org, Jun 12 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 13 2017

Cc: bmeu...@chromium.org
Owner: bmeu...@chromium.org

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

Hi bmeurer@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 : bmeurer
  Commit : af76779aa3064a8d5ce8f7b88836110f3de4cfa0
  Date   : Thu Jun 08 18:31:59 2017
  Subject: [builtins] Start refactoring the Apply builtin.

Bisect Details
  Configuration: win_perf_bisect
  Benchmark    : media.tough_video_cases_tbmv2
  Metric       : memory:chrome:renderer_processes:reported_by_chrome:v8:effective_size_avg/video.html?src_tulip2.vp9.webm_seek
  Change       : 11.07% | 4734976.0 -> 5259264.0

Revision                           Result              N
chromium@478301                    4734976 +- 0.0      6      good
chromium@478310                    4734976 +- 0.0      6      good
chromium@478310,v8@af2a8eae63      4734976 +- 0.0      6      good
chromium@478310,v8@ae947e26fe      4734976 +- 0.0      6      good
chromium@478310,v8@af76779aa3      5259264 +- 0.0      6      bad       <--
chromium@478310,v8@381f7da02c      5259264 +- 0.0      6      bad
chromium@478310,v8@195eab4619      5259264 +- 0.0      6      bad
chromium@478311                    5259264 +- 0.0      6      bad
chromium@478312                    5259264 +- 0.0      6      bad
chromium@478313                    5259264 +- 0.0      6      bad
chromium@478315                    5259264 +- 0.0      6      bad
chromium@478319                    5259264 +- 0.0      6      bad
chromium@478336                    5259264 +- 0.0      6      bad
chromium@478371                    5259264 +- 0.0      6      bad
chromium@478441                    5259264 +- 0.0      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=video.html.src.tulip2.vp9.webm.seek media.tough_video_cases_tbmv2

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8976968489503084544

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5769798346080256


| 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!
Cc: petermarshall@chromium.org jgruber@chromium.org
Status: WontFix (was: Untriaged)
The CL splits one large builtin into 4 smaller builtins, that can be reused appropriately, as part of a larger refactoring.
Are you saying that the 10% increase in memory:chrome:renderer_processes:reported_by_chrome:v8:effective_size_avg is acceptable given the other benefits of the refactor? I follow what the CL does, but I'm confused about WontFix resolution.
Owner: hpayer@chromium.org
Status: Available (was: WontFix)
I should have been more explicit I guess. The concrete CL doesn't really matter here. The underlying issue is that we need a couple of bytes in addition in code space, and hence we cross the boundary and need to allocate another block of pages for code space, which explains the 10% increase. There's not much I can do about this, because as said, it could be any CL, and it could also easily drop again.

Maybe the memory team would/should look into this. Assigning this to hpayer@ for this. Feel free to ignore/wontfix.
Cc: dalecur...@chromium.org
Thanks for explaining. Adding current media sheriff to CC
Components: Speed
Labels: Performance-Memory
This is a decent increase, ~500kb / 10% in memory; so +speed team and appropriate tagging to figure out if more should be done here.
Cc: benhenry@chromium.org
+benhenry see #8
Cc: -bmeu...@chromium.org hpayer@chromium.org
Owner: mlippautz@chromium.org
Assigning to the current memory sheriff
Cc: mlippautz@chromium.org
Owner: bmeu...@chromium.org
Status: WontFix (was: Available)
A page in V8 is 512kb, so adding another page will result in increases in effective size (=committed) of that range. As #6 already pointed out: If we are close to the page boundary then any other CL dealing with code could cause this regression too.

For more context: The real issue here is that the current strategy is moving functionality to builtins for performance (speed) reasons which doesn't play well with eagerly loading them. The V8 team is already aware of this issue and is investigating how to properly address this issue.

Short term, there's nothing to be done here. Assigning back to CL owner and marking as WontFix.

Assuming we'd need a larger investigation, but are there pieces of code we can pull out to avoid having to page so much additional memory? Have we tried decreasing the page size so we don't have to pull in so much? TBH - 500k is not a lot in the whole scheme of things, but if that's +500k per tab, that would be very bad.
Decreasing the page size in V8 comes with all sorts of side effects, e.g., large object size limit, that potentially triggers a rather huge change in all benchmarks. The last time we did this we decreases from 1M to 512k. This already came with some regressions on performance-sensitive benchmarks but we took the hit for reducing memory. We are investigating to further reduce the page size but this is not something that can be done quickly.

The 500k here is per tab as it's per Isolate. There is nothing we can do short term here though as any other change might as well trigger allocating a new page. 

As for mitigations: We already shrink pages (1b4934b) after loading snapshots. This was broken and just fixed recently (2 days ago) and shows up on the graphs as reductions.

Lazy loading of builtins is subject to further investigations in Q3+ afaik.

Sign in to add a comment