Issue metadata
Sign in to add a comment
|
2.2% regression in system_health.memory_desktop at 410410:410509 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Aug 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9004795884397003600
,
Aug 10 2016
=== Auto-CCing suspected CL author rune@opera.com === Hi rune@opera.com, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Style invalidation support for :first/last/only-child. Author : rune Commit description: Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed issue 245914 by looking at next/previous element, not node, to figure out if we are the first. The code in checkForSiblingStyleChanges could be restructured quite a bit and made simpler now have changedElement (changedNode => changeElement since the input is always an element). BUG=245914 Review-Url: https://codereview.chromium.org/2229503002 Cr-Commit-Position: refs/heads/master@{#410472} Commit : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05 Date : Mon Aug 08 22:08:09 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@410470 23432.0 0.0 8 good chromium@410471 23432.0 0.0 5 good chromium@410472 23918.0 196.553 8 bad <-- chromium@410473 23873.6 246.862 5 bad chromium@410476 23873.6 246.862 5 bad chromium@410480 23984.0 0.0 5 bad Bisect job ran on: win_8_perf_bisect Bug ID: 635922 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox Relative Change: 2.36% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2110 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004795884397003600 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=6191668521336832 | 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!
,
Aug 12 2016
I don't understand how my change could possibly affect font cache sizes. petrcermak@: I see your name in the log for the relevant tests. Any ideas how to proceed?
,
Aug 12 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9004545383173362480
,
Aug 12 2016
rune: I'm going to run one more bisect to confirm that this is due to your patch. Assuming the bisect confirms the current result, then I suggest you have a look at the traces generated by the bisect before and after your patch (I'll help you get a hold of them). If there's nothing strange, then I think we should mark this as WontFix, because it's a <1 KiB regression.
,
Aug 12 2016
Btw, the test does not have an owner according to appspot. Perhaps you could add yourself or the appropriate owner to the test(s)? The owner list seems to be Google internal. At least, it goes to go/ if I try to "edit owners" from appspot.
,
Aug 12 2016
What do you mean by "according to appspot"?
,
Aug 12 2016
The first url has an empty "owners:" while the second says "owners: rune@opera.com": https://chromeperf.appspot.com/group_report?bug_id=635922 https://chromeperf.appspot.com/group_report?bug_id=618674 I don't know where the owners are recorded, but I gathered they could be changed through "Other Pages => Edit Test Owners".
,
Aug 12 2016
My name is in the spreadsheet. +cc sullivan: Any idea why my name doesn't show up in https://chromeperf.appspot.com/group_report?bug_id=635922?
,
Aug 12 2016
The dashboard 'owners' feature is pretty broken, we need to redo it. Currently the spreadsheet (go/perf-owners) is manual.
,
Aug 12 2016
I have access to: docs.google.com/spreadsheets/d/1R_1BAOd3xeVtR0jn6wB5HHJ2K25mIbKp3iIRQKkX38o/ Does go/perf-owners redirect to that sheet, or is it different?
,
Aug 12 2016
Yes, it's that sheet.
,
Aug 12 2016
Thanks! It would be good if that link on appspot was accessible to non-Googlers.
,
Aug 12 2016
Re #14: I replaced the outdated owners on appspot with a link to the spreadsheet.
,
Aug 13 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Style invalidation support for :first/last/only-child. Author : rune Commit description: Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed issue 245914 by looking at next/previous element, not node, to figure out if we are the first. The code in checkForSiblingStyleChanges could be restructured quite a bit and made simpler now have changedElement (changedNode => changeElement since the input is always an element). BUG=245914 Review-Url: https://codereview.chromium.org/2229503002 Cr-Commit-Position: refs/heads/master@{#410472} Commit : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05 Date : Mon Aug 08 22:08:09 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@410470 23432.0 0.0 8 good chromium@410471 23443.0 31.1127 8 good chromium@410472 23873.6 246.862 5 bad <-- chromium@410474 23873.6 246.862 5 bad chromium@410478 23873.6 246.862 5 bad chromium@410486 23873.6 246.862 5 bad chromium@410509 23891.2 207.507 5 bad Bisect job ran on: win_8_perf_bisect Bug ID: 635922 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox Relative Change: 1.96% Score: 95.0 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2114 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004545383173362480 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5854200659968000 | 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!
,
Aug 15 2016
The second bisect still points to my change. The only way I can think that my change could affect the font cache size would be if some page in the set re-calculate style for fewer elements in the presence of matching :first-child/:last-child/:only-child selectors, and that some fonts' data would not be accessed during the recalc that otherwise would have, and that this somehow affects the font cache. I don't have access to any Windows machines to test with.
,
Aug 15 2016
We tear down the browser after every single page, so the regression would have to happen within the Dropbox page. To confirm your hypothesis, could you please prepare a patch that does some sort of instrumentation and submit it as a tryjob to the windows bots?
,
Aug 15 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9004241097308193728
,
Aug 16 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Style invalidation support for :first/last/only-child. Author : rune Commit description: Got rid of SubtreeStyleChanges for those pseudo classes. Also fixed issue 245914 by looking at next/previous element, not node, to figure out if we are the first. The code in checkForSiblingStyleChanges could be restructured quite a bit and made simpler now have changedElement (changedNode => changeElement since the input is always an element). BUG=245914 Review-Url: https://codereview.chromium.org/2229503002 Cr-Commit-Position: refs/heads/master@{#410472} Commit : 798b5cdff6f50fa0d19a198f5f6e3f57425fbf05 Date : Mon Aug 08 22:08:09 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@410470 23439.3 25.4034 12 good chromium@410471 23432.0 0.0 5 good chromium@410472 23873.6 246.862 5 bad <-- chromium@410474 23984.0 0.0 5 bad chromium@410478 23800.0 271.786 12 bad chromium@410486 23987.0 8.48528 8 bad chromium@410509 23873.6 246.862 5 bad Bisect job ran on: win_8_perf_bisect Bug ID: 635922 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop Test Metric: load_tools-memory:chrome:all_processes:reported_by_chrome:font_caches:effective_size_avg/load_tools_dropbox Relative Change: 1.88% Score: 98.0 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisect/builds/2116 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004241097308193728 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5286656484048896 | 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!
,
Aug 16 2016
I tried reverting: crrev.com/798b5cdf (410472) crrev.com/44689f75 crrev.com/1ba55572 The two extra reverts were changes on top of 410472, so necessary for clean revert. Then ran it the win-8 try-bot: https://codereview.chromium.org/2254443002/ The reverts did not cause any improvements for font cache avg size for dropbox: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-08-16_08-10-20
,
Aug 17 2016
rune: Thanks a lot for the investigaiton. Could you please try reverting all the way to 410471 (before your patch) and then running the tryjob (to check that the perf bot can actually reproduce the regression)?
,
Aug 30 2016
,
Sep 16 2016
Temporarily declaring bankruptcy on the *desktop* system health benchmark. The number of alerts became unmanageable and the overall process needs to be improved to make it sustainable. The alerts have been turned off and I'm archiving the outstanding regressions. Note: this is just about desktop, the mobile system health stays up. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by petrcermak@chromium.org
, Aug 9 2016