Issue metadata
Sign in to add a comment
|
10%-11.1% regression in media.tough_video_cases_tbmv2 at 478302:478441 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 12 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8976968489503084544
,
Jun 13 2017
=== 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!
,
Jun 13 2017
The CL splits one large builtin into 4 smaller builtins, that can be reused appropriately, as part of a larger refactoring.
,
Jun 13 2017
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.
,
Jun 13 2017
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.
,
Jun 13 2017
Thanks for explaining. Adding current media sheriff to CC
,
Jun 13 2017
This is a decent increase, ~500kb / 10% in memory; so +speed team and appropriate tagging to figure out if more should be done here.
,
Jun 14 2017
+benhenry see #8
,
Jun 14 2017
Assigning to the current memory sheriff
,
Jun 14 2017
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.
,
Jun 14 2017
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.
,
Jun 14 2017
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 |
|||||||||||||||||||||||
Comment 1 by chcunningham@chromium.org
, Jun 12 2017