Issue metadata
Sign in to add a comment
|
2.3%-24.3% regression in system_health.memory_mobile at 434672:435043 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Dec 1 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8994456345729661344
,
Dec 2 2016
=== PERF REGRESSION === === Auto-CCing suspected CL author ishell@chromium.org === Hi ishell@chromium.org, the bisect results pointed to your CL, please take a look at the results. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : [ic] Use validity cells to protect keyed element stores against object's prototype chain modifications. Author : ishell Commit description: ... instead of clearing of all the KeyedStoreICs which didn't always work. BUG= chromium:662907 , v8:5561 TBR=verwaest@chromium.org, bmeurer@chromium.org Review-Url: https://codereview.chromium.org/2534613002 Cr-Commit-Position: refs/heads/master@{#41332} Commit : a39522f44f7e0be4686831688917e9675255dcaf Date : Mon Nov 28 22:56:52 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@434671 1857212 14883.3 5 good chromium@434857 1860787 17876.0 5 good chromium@434950 2153655 349.28 5 good chromium@434974 2153694 270.679 5 good chromium@434977 2153686 289.198 5 good chromium@434978 2153581 236.129 5 good chromium@434978,v8@a39522f44f 2154174 289.198 5 bad <-- chromium@434978,v8@bc1a3820c2 2154191 323.055 5 bad chromium@434978,v8@18eda7024b 2154234 270.679 5 bad chromium@434978,v8@4e7571a5a9 2264323 220.356 5 bad chromium@434979 2269733 236.129 5 bad chromium@434980 2269812 374.783 8 bad chromium@434986 2269854 357.592 8 bad chromium@434997 2269812 373.352 8 bad chromium@435043 2269740 268.328 5 bad Bisect job ran on: android_nexus5_perf_bisect Bug ID: 670352 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.games.lazors system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_games/load_games_lazors Relative Change: 22.21% Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/4393 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8994456345729661344 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5272040024571904 | 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 Tests>AutoBisect. Thank you!
,
Dec 2 2016
Setting correct status.
,
Dec 2 2016
This bisect went wrong. The above report looks weird: mean goes from 1860787 to 2153655 in a completely different commit, but stddev changes a lot. There seems to be a second change that is between 434978 and 434979 at the v8 roll, but the revision marked as bad is just as good as the last good one. What is clearly showing that bisect was wrong is that the marked culprit a39522f was revert within the range of the roll: https://chromium.googlesource.com/v8/v8/+log/2ef8719e..928e6b56
,
Dec 5 2016
+dtu
So the weird comparison in question (chromium@434950)
Comparison vs Good:
{
"result": {
"U": 0,
"p": 0.010909498364269243,
"significance": "NEED_MORE_DATA"
},
"sampleA": [
2153528,
2153864,
2153828,
2153528,
2153528
],
"sampleB": [
1861540,
1865832,
1865596,
1845136,
1865832
]
}
Comparison vs Bad:
{
"result": {
"U": 0,
"p": 0.008784000372477396,
"significance": "REJECT"
},
"sampleA": [
2153528,
2153864,
2153828,
2153528,
2153528
],
"sampleB": [
2269680,
2269680,
2269680,
2269680,
2269980
]
}
In revision_state.py I think the bisect decides that if it only gets one SIGNIFICANTLY_DIFFERENT, in this case from BAD, it assigns the revision to the opposite, in this case GOOD. Maybe it should also confirm that the other revision is the same? Dave, wdyt?
,
Dec 5 2016
The issue I see in #6 is that five runs is not enough even in many clear comparisons (e.g. comparison vs good above) there is a bug somewhere to raise the starting reps to 8.
,
Dec 5 2016
Will upping the default # of runs eliminate the issue?
,
Dec 5 2016
Upping it would have made both revisions "REJECT" and in that case the bisect logic is to bisect into the one with the greater difference between the means of the samples.
,
Dec 5 2016
Gotcha ok. Seems like we could make it clearer than multiple regressions were detected and this is the one we chose to dive in to?
,
Dec 5 2016
I made a change to try this: https://chromium-review.googlesource.com/416380
,
Dec 6 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8993989322084320000
,
Dec 6 2016
=== PERF REGRESSION === === Auto-CCing suspected CL author yukishiino@chromium.org === Hi yukishiino@chromium.org, the bisect results pointed to your CL, please take a look at the results. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : binding: Makes non-cross-origin-accessible attrs be accessor props. Author : yukishiino Commit description: Makes non-cross-origin accessible attributes be accessor properties. [CachedAccessor] already made window.document an accessor property, and this CL doesn't affect it. Cross-origin accessible attributes remain being data properties. The previous attempt was http://crrev.com/1380503002 BUG=475556 Review-Url: https://codereview.chromium.org/2506393004 Cr-Commit-Position: refs/heads/master@{#434942} Commit : 5b2adfa4d4e2fb6b74d177625605dcece20a40b7 Date : Tue Nov 29 09:53:57 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@434671 1859646 4855.69 8 good chromium@434857 1860033 24780.3 8 good chromium@434904 1858460 26769.6 8 good chromium@434927 1854800 25700.8 8 good chromium@434939 1865666 275.797 8 good chromium@434941 1858431 26521.9 8 good chromium@434942 2153641 316.855 5 bad <-- chromium@434945 2153626 289.198 5 bad chromium@434950 2153641 277.613 5 bad chromium@435043 2269800 298.315 5 bad Bisect job ran on: android_nexus5_perf_bisect Bug ID: 670352 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.games.lazors system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_games/load_games_lazors Relative Change: 22.06% Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/4412 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8993989322084320000 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5278391375233024 | 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 Tests>AutoBisect. Thank you!
,
Dec 7 2016
Question to V8 team, 300 kbytes increase of memory usage is expected from PoV of V8? I changed 155 attributes with 291 get/set functions. The suspicious CL (http://crrev.com/1380503002) made Window's DOM attributes be accessor properties (were data properties), and it should increase some amount of memory usage because we need 'get' and 'set' functions in addition. number of affected attributes = 155 number of get/set functions = 291 Thus, my change expects the increase of memory usage as: function size * 291 I don't know the exact size of v8::FunctionTemplate. Seeing #c13, it seems we have 300 kbytes increase. Is this expected from PoV of V8?
,
Dec 7 2016
A FunctionTemplate is about 168 bytes + any other data that hangs off of it (but it shouldn't be 100k) Ulan, do you know what "v8:effective_size_avg" is? is there a metric that would help us understand what exactly went up?
,
Dec 7 2016
That metric is the committed size of v8 heap + v8 malloc. One can drill down by looking at v8:heap:... In this case it looks like that we are allocating 1MB more large objects than before: https://chromeperf.appspot.com/report?sid=4d30c50820f5367209b0c83484896e4883c32a30a99aa21aacedc7d95ddf3c63&rev=434987
,
Dec 9 2016
ulan@, can I hand over this issue to you? I'm poor in this area to investigate what's going on in V8. If +1MB memory usage was not a surprise, I'm fine to close this issue as WontFix.
,
Dec 9 2016
If 1MB increase is unexpected then I would revert the CL until we understand the cause. My guesses: 1) a memory leak. 2) we went over a threshold for growing the backing store of some object or some table. Since the diff is in large object space, I would suggest to add a call to LargeObjectSpace::ReportStatistics() after V8 GC (Heap::CollectGarbage) and find what object has appeared (or grown) after the CL. Then you can print the object using Object::Print and see what kind of object it is. This will help us understand what's going on. Assigning back to you, feel free to ask question if something is not clear.
,
Dec 12 2016
Hi ulan@, I prepared a CL: https://crrev.com/2568813002 Is this what you suggested? Am I walking the right way? Also, could you tell me how to run the perf test for the large object space (on Android)? If you know concrete command lines, that's very helpful.
,
Dec 12 2016
Yes, that looks right. Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.games.lazors system_health.memory_mobile The output should appear in adb logcat
,
Dec 22 2016
I've finally run some tests, but didn't observe any difference in LargeObjectSpace::ReportStatistics(). I observed a regression of memory:chrome:...:v8:effective_size with my CL applied, however there is no difference in LargeObjectSpace::ReportStatistics(). Results are like this (debug build since ReportStatistics() is supported only in debug build): 12-22 23:03:37.984 31683 31700 I v8 : size: 98304 12-22 23:03:37.984 31683 31700 I v8 : number of objects 3, size of objects 1280 12-22 23:03:37.984 31683 31700 I v8 : 12-22 23:03:37.984 31683 31700 I v8 : Object Histogram: 12-22 23:03:37.984 31683 31700 I v8 : CODE_TYPE 3 ( 1280 bytes) I checked with release build just in case (without histogram): 12-22 23:27:03.220 2002 2021 I v8 : size: 98304 12-22 23:27:03.220 2002 2021 I v8 : number of objects 3, size of objects 1088 But results are same between with and without my CL. ulan@, why did you think the diff is in large object space? Seeing the graphs at: https://chromeperf.appspot.com/report?sid=784b96aea190f0d10dbf1b988cbd02a19f6d73138d48653fa450b881095d3e5e&start_rev=427255&end_rev=436192 https://chromeperf.appspot.com/report?sid=69a0eb1572dc7b7e263972d2716979b717b6eb7e60ad66c61f1b59dfcecb02e7&start_rev=427255&end_rev=436192 I'm now guessing that it's ulan> 2) we went over a threshold for growing the backing store of some object or some table. because it bumped up to the previous level. Plus, my CL didn't introduce any new mechanism. Accessor properties have already been used for long time except for Window and Location. I just applied it to Window and Location. It's unlikely to be a leak. What do you think?
,
Jan 10 2017
Friendly ping? Anyone has any thoughts and/or concerns? If there is no objection, I will close this issue as WontFix.
,
Jan 10 2017
Sorry for delay. I am back from holidays. As I said in #18. The graphs show 1MB increase in large object space in android-nexus9/memory.top_10_mobile_stress / memory:chrome:all_processes:reported_by_chrome:v8:heap:large_object_space:effective_size_avg / foreground / http_www_amazon_com_gp_aw_s_k_nexus https://chromeperf.appspot.com/report?sid=4d30c50820f5367209b0c83484896e4883c32a30a99aa21aacedc7d95ddf3c63&rev=434987 Looks like it did not reproduce for you. What device and benchmark did you try? I think we shouldn't close as won't fix unless we figure out that this is definitely not a memory leak. If it is a memory leak it will bite us when Chrome hits stable.
,
Jan 10 2017
I tested with Nexus 7 (2013). The benchmark was load.games.lazors system_health.memory_mobile as you wrote at #20. https://chromeperf.appspot.com/report?sid=001a74298b3e2cae616955d7b84cf390d1dbc8951e87c805df94c48818fb34cc&start_rev=434432&end_rev=437363 Seeing this graph, it should be reproducible with Nexus 7 v2. Was I using a wrong benchmark or was the CL wrong?
,
Jan 10 2017
In #20 I took the command line from bisect result in #13. Maybe that particular story doesn't reproduce large object space regression. From the graph you linked, it looks like "--story-filter=load:search:yandex memory.top_10_mobile_stress" should reproduce the large object space regression on nexus7v2
,
Jan 11 2017
Thanks for helping me investigate this issue. "--story-filter='http_yandex_ru_touchsearch_text_science' memory.top_10_mobile_stress" worked with Nexus 7v2, and I bisected to investigate which CL increased the large object space. The results with release build in reverse chronological order are as follows. // ToT as of now commit 1a7740c44a6c4d03423c4b5ff7d746e1a1b7363e 01-11 21:08:25.321 17492 17509 I v8 : size: 98304 01-11 21:08:25.321 17492 17509 I v8 : number of objects 3, size of objects 1088 // v8-autoroll commit 4bd1b61eb337d4c6e1d319141471b0238ed7287d 01-11 23:00:58.366 23792 23810 I v8 : size: 65536 01-11 23:00:58.366 23792 23810 I v8 : number of objects 2, size of objects 736 // just before the above v8-autoroll commit 50ccef041685519550b26723c1e6bec74b9f9fc3 01-11 23:16:48.227 25433 25460 I v8 : size: 0 01-11 23:16:48.227 25433 25460 I v8 : number of objects 0, size of objects 0 // My CL under suspicion commit 5b2adfa4d4e2fb6b74d177625605dcece20a40b7 01-11 21:40:33.719 18851 18868 I v8 : size: 0 01-11 21:40:33.719 18851 18868 I v8 : number of objects 0, size of objects 0 With the above results, it looks that V8's change caused the increase of the large object space. ulan@, could you take a look into https://chromium.googlesource.com/chromium/src/+/4bd1b61eb337d4c6e1d319141471b0238ed7287d ?
,
Jan 11 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8990754221836383632
,
Jan 11 2017
Thanks for bisecting. Looks like the alert has two different regressions one in #13 and one in large object space. I started bisect for the large object space regression.
,
Jan 11 2017
Do you have any ideas for #13 about what I can do for it? It's by design and required by the spec that I increased the number of instances of v8::Function (and v8::FunctionTemplate) in order to make Window's attributes accessor-properties. Seeing the following graphs (the same ones at #21), https://chromeperf.appspot.com/report?sid=784b96aea190f0d10dbf1b988cbd02a19f6d73138d48653fa450b881095d3e5e&start_rev=427255&end_rev=436192 https://chromeperf.appspot.com/report?sid=69a0eb1572dc7b7e263972d2716979b717b6eb7e60ad66c61f1b59dfcecb02e7&start_rev=427255&end_rev=436192 it's not too bad compared to the previous level, I think. Is this acceptable? What do you think?
,
Jan 11 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : jgruber Commit : 4e7571a5a9bb8563ae93e0aff48c08bead765b14 Date : Tue Nov 29 09:03:18 2016 Subject: [regexp] Migrate @@match to TurboFan Bisect Details Configuration: android_nexus7_perf_bisect Benchmark : memory.top_10_mobile_stress Metric : memory:chrome:all_processes:reported_by_chrome:v8:heap:large_object_space:allocated_objects_size_avg/foreground Revision Result N chromium@434955 0.0 +- 0.0 60 good chromium@434973 0.0 +- 0.0 60 good chromium@434978 0.0 +- 0.0 60 good chromium@434978,v8@18eda7024b 0.0 +- 0.0 60 good chromium@434978,v8@9c0e2a6723 0.0 +- 0.0 60 good chromium@434978,v8@4e7571a5a9 736.0 +- 0.0 60 bad <-- chromium@434979 736.0 +- 0.0 60 bad chromium@434980 736.0 +- 0.0 60 bad chromium@434982 736.0 +- 0.0 60 bad chromium@434991 736.0 +- 0.0 60 bad 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/8990754221836383632 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6472223256215552 | 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 Tests>AutoBisect. Thank you!
,
Jan 12 2017
> Do you have any ideas for #13 about what I can do for it? > It's by design and required by the spec that I increased the number of instances of v8::Function (and v8::FunctionTemplate) in order to make Window's attributes accessor-properties. I don't know. Maybe devtools heapsnapshot or detailed memory dump before/after the CL can help verify that the memory increase comes from the v8::Function increase? Since we found the culprit for large object space regression, it is probably ok to "won't fix" this issue if you're confident that there are no memory leaks. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by lanwei@chromium.org
, Dec 1 2016