Issue metadata
Sign in to add a comment
|
3.6%-5.4% regression in system_health.memory_mobile at 441980:442051 |
||||||||||||||||||||||
Issue descriptionTrying bisect.
,
Jan 10 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8990898237010033648
,
Jan 10 2017
=== Auto-CCing suspected CL author bokan@chromium.org === Hi bokan@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 : bokan Commit : 18940be9f2a3912efbdbed71863a7749ed243e9a Date : Fri Jan 06 18:53:20 2017 Subject: Use full viewport height for layout if URL bar is locked shown or hidden. Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:all_processes:reported_by_chrome:skia:effective_size_avg/browse_news/browse_news_cnn Change : 5.11% | 3102261.33333 -> 3260750.66667 Revision Result N chromium@441984 3102261 +- 2655.68 6 good chromium@441993 3113607 +- 38910.0 6 good chromium@441994 3089147 +- 71421.9 6 good chromium@441995 3257407 +- 82638.7 6 bad <-- chromium@441996 3228248 +- 188706 9 bad chromium@441998 3257131 +- 136747 6 bad chromium@442002 3263300 +- 96322.8 6 bad chromium@442020 3260751 +- 115394 6 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 --story-filter=browse.news.cnn system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8990898237010033648 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5891440464363520 | 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 10 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8990844874997866592
,
Jan 10 2017
I don't see how my CL might have increased memory usage. Kicking off a new bisect to confirm.
,
Jan 10 2017
,
Jan 10 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : bokan Commit : 18940be9f2a3912efbdbed71863a7749ed243e9a Date : Fri Jan 06 18:53:20 2017 Subject: Use full viewport height for layout if URL bar is locked shown or hidden. Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:all_processes:reported_by_chrome:skia:effective_size_avg/browse_news/browse_news_cnn Change : 4.54% | 3102077.33333 -> 3242818.66667 Revision Result N chromium@441979 3102077 +- 1580.75 6 good chromium@441988 3109155 +- 40239.4 6 good chromium@441993 3109203 +- 40292.3 6 good chromium@441994 3102395 +- 2540.9 6 good chromium@441995 3270608 +- 6722.47 6 bad <-- chromium@441997 3278077 +- 37587.2 6 bad chromium@442015 3324837 +- 232011 5 bad chromium@442051 3242819 +- 96892.6 6 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 --story-filter=browse.news.cnn system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8990844874997866592 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6335242387849216 | 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 10 2017
Try reverting your change and seeing if this metric improves to confirm.
,
Jan 10 2017
Two bisects pointed to it so I suspect it is my change. I'll take a closer look locally this week.
,
Jan 23 2017
,
Jan 24 2017
,
Jan 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38992dee686ca58d5f1a61d44ef3e534b8c2967c commit 38992dee686ca58d5f1a61d44ef3e534b8c2967c Author: bokan <bokan@chromium.org> Date: Tue Jan 24 23:26:25 2017 Changing URL bar constraints only calls performResize on Hidden <-> Both With the change to the URL bar not causing a vertical resize on showing/hiding, we keep the layout height sized "as if" the URL bar was always shown. However, for fullscreen scenarios, since the URL bar is locked to a hidden state there's no need to keep it static to the smaller size so we use the full viewport height instead. This means that when we change the URL bar constraints we may need to change the layout height. In most cases, we can rely on a Resize event being sent to the renderer kicking off the layout size change. The one exception here is from a "Locked Hidden" to "Unlocked" state, or vice-versa. That's because the URL bar's state might not change (it can remain hidden) but the layout size should change. Note that going from "Locked Hidden" -> "Shown" doesn't have this problem as the URL bar state will change necessarily. Going from "Unlocked" -> "Shown" may or may not change the URL bar state, but it will not change the layout height since we use the "Shown" height for layout height when the URL bar is "Unlocked". Before this patch, we were overly-aggressive in triggering performResize which caused us to perform extra work as the page went from "Locked Shown" to "Unlocked", as happens when the page is navigated. This caused a minor memory regression. This patch performs the resize only when needed, on going from "Locked Hidden" to "Unlocked" or vice-versa. BUG= 679546 Review-Url: https://codereview.chromium.org/2656633003 Cr-Commit-Position: refs/heads/master@{#445855} [modify] https://crrev.com/38992dee686ca58d5f1a61d44ef3e534b8c2967c/third_party/WebKit/Source/web/WebViewImpl.cpp
,
Jan 24 2017
The patch above should fix the regression - I'll confirm once it cycles through the perf bots. It's too late to merge to 56 but I'll merge it back go 57 once it hits Canary.
,
Jan 26 2017
Weird, the bots showed no improvements. I've retried repeatedly locally - the results I'm seeing have some variation and the occasional puzzling result but there seems to be a clear difference when I revert my latest patch and ToT. I'm going to revert both my fix and the original patch in ToT to confirm.
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/13876a1d16f4e9a09ffacebcd3b9df0a21a2e55b commit 13876a1d16f4e9a09ffacebcd3b9df0a21a2e55b Author: bokan <bokan@chromium.org> Date: Thu Jan 26 17:34:02 2017 Revert of Changing URL bar constraints only calls performResize on Hidden <-> Both (patchset #1 id:1 of https://codereview.chromium.org/2656633003/ ) Reason for revert: Didn't help on the perf bots. Temporarily reverting along with orignial patch to investigate. Original issue's description: > Changing URL bar constraints only calls performResize on Hidden <-> Both > > With the change to the URL bar not causing a vertical resize on showing/hiding, > we keep the layout height sized "as if" the URL bar was always shown. However, > for fullscreen scenarios, since the URL bar is locked to a hidden state there's > no need to keep it static to the smaller size so we use the full viewport > height instead. > > This means that when we change the URL bar constraints we may need to change > the layout height. In most cases, we can rely on a Resize event being sent to > the renderer kicking off the layout size change. The one exception here is from > a "Locked Hidden" to "Unlocked" state, or vice-versa. That's because the URL > bar's state might not change (it can remain hidden) but the layout size should > change. > > Note that going from "Locked Hidden" -> "Shown" doesn't have this problem as > the URL bar state will change necessarily. Going from "Unlocked" -> "Shown" may > or may not change the URL bar state, but it will not change the layout height > since we use the "Shown" height for layout height when the URL bar is > "Unlocked". > > Before this patch, we were overly-aggressive in triggering performResize which > caused us to perform extra work as the page went from "Locked Shown" to > "Unlocked", as happens when the page is navigated. This caused a minor memory > regression. This patch performs the resize only when needed, on going from > "Locked Hidden" to "Unlocked" or vice-versa. > > BUG= 679546 > > Review-Url: https://codereview.chromium.org/2656633003 > Cr-Commit-Position: refs/heads/master@{#445855} > Committed: https://chromium.googlesource.com/chromium/src/+/38992dee686ca58d5f1a61d44ef3e534b8c2967c TBR=aelias@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 679546 Review-Url: https://codereview.chromium.org/2659483003 Cr-Commit-Position: refs/heads/master@{#446362} [modify] https://crrev.com/13876a1d16f4e9a09ffacebcd3b9df0a21a2e55b/third_party/WebKit/Source/web/WebViewImpl.cpp
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe commit 01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe Author: bokan <bokan@chromium.org> Date: Fri Jan 27 17:01:54 2017 Revert of Use full viewport height for layout if URL bar is locked shown or hidden. (patchset #2 id:20001 of https://codereview.chromium.org/2614993002/ ) Reason for revert: Temporarily reverting to see if it fixes the memory regression in the linked bug. BUG= 679546 Original issue's description: > Use full viewport height for layout if URL bar is locked shown or hidden. > > With inert-top-controls turned on by default, we don't change the layout height > when the top controls are normally hidden. That is, if a user hides the URL bar > by scrolling, the page won't resize to use the newly available space. It'll lay > out as though the URL bar was still showing. > > This patch makes an exception for cases where Chrome locks the URL bar in the > hidden state. e.g. Fullscreen or WebApp mode. In those cases, since the URL bar > can't be shown at all, there's no reason not to use the full height. > > BUG= 678649 > > Review-Url: https://codereview.chromium.org/2614993002 > Cr-Commit-Position: refs/heads/master@{#441995} > Committed: https://chromium.googlesource.com/chromium/src/+/18940be9f2a3912efbdbed71863a7749ed243e9a TBR=aelias@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 678649 Review-Url: https://codereview.chromium.org/2657773005 Cr-Commit-Position: refs/heads/master@{#446693} [modify] https://crrev.com/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe/third_party/WebKit/Source/core/frame/BrowserControls.h [modify] https://crrev.com/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe/third_party/WebKit/Source/web/WebViewImpl.cpp [modify] https://crrev.com/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe/third_party/WebKit/Source/web/WebViewImpl.h [modify] https://crrev.com/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe/third_party/WebKit/Source/web/tests/BrowserControlsTest.cpp
,
Jan 30 2017
So I reverted my patch and it didn't make a difference so it looks to be unrelated - I'm relanding now. I'm kicking off yet another bisect but I suspect this metric might be too noisy and regression too small to get a good read.
,
Jan 30 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8989020861030361904
,
Jan 30 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : bokan Commit : 18940be9f2a3912efbdbed71863a7749ed243e9a Date : Fri Jan 06 18:53:20 2017 Subject: Use full viewport height for layout if URL bar is locked shown or hidden. Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:all_processes:reported_by_chrome:skia:effective_size_avg/browse_news/browse_news_cnn Change : 4.27% | 3111840.0 -> 3244666.0 Revision Result N chromium@441979 3111840 +- 54758.1 9 good chromium@441988 3088856 +- 73189.6 6 good chromium@441993 3095389 +- 82232.9 9 good chromium@441994 3089464 +- 74017.6 6 good chromium@441995 3230541 +- 303479 14 bad <-- chromium@441997 3234614 +- 212504 9 bad chromium@442015 3279680 +- 39979.2 6 bad chromium@442051 3244666 +- 128493 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=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.news.cnn system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8989020861030361904 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5823475308036096 | 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 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9427bfd430073a9caa0770ab6653b040520a8aab commit 9427bfd430073a9caa0770ab6653b040520a8aab Author: bokan <bokan@chromium.org> Date: Tue Jan 31 01:37:01 2017 Reland of Use full viewport height for layout if URL bar is locked shown or hidden. (patchset #1 id:1 of https://codereview.chromium.org/2657773005/ ) Reason for revert: Relanding since the graph in the original bug didn't get any better, ruling out this CL. Original issue's description: > Revert of Use full viewport height for layout if URL bar is locked shown or hidden. (patchset #2 id:20001 of https://codereview.chromium.org/2614993002/ ) > > Reason for revert: > Temporarily reverting to see if it fixes the memory regression in the linked bug. > BUG= 679546 > > Original issue's description: > > Use full viewport height for layout if URL bar is locked shown or hidden. > > > > With inert-top-controls turned on by default, we don't change the layout height > > when the top controls are normally hidden. That is, if a user hides the URL bar > > by scrolling, the page won't resize to use the newly available space. It'll lay > > out as though the URL bar was still showing. > > > > This patch makes an exception for cases where Chrome locks the URL bar in the > > hidden state. e.g. Fullscreen or WebApp mode. In those cases, since the URL bar > > can't be shown at all, there's no reason not to use the full height. > > > > BUG= 678649 > > > > Review-Url: https://codereview.chromium.org/2614993002 > > Cr-Commit-Position: refs/heads/master@{#441995} > > Committed: https://chromium.googlesource.com/chromium/src/+/18940be9f2a3912efbdbed71863a7749ed243e9a > > TBR=aelias@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 678649 > > Review-Url: https://codereview.chromium.org/2657773005 > Cr-Commit-Position: refs/heads/master@{#446693} > Committed: https://chromium.googlesource.com/chromium/src/+/01f12135d93ddd4f3daf304f8a3b86c4cd7c6dbe TBR=aelias@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 679546 Review-Url: https://codereview.chromium.org/2663823002 Cr-Commit-Position: refs/heads/master@{#447155} [modify] https://crrev.com/9427bfd430073a9caa0770ab6653b040520a8aab/third_party/WebKit/Source/core/frame/BrowserControls.h [modify] https://crrev.com/9427bfd430073a9caa0770ab6653b040520a8aab/third_party/WebKit/Source/web/WebViewImpl.cpp [modify] https://crrev.com/9427bfd430073a9caa0770ab6653b040520a8aab/third_party/WebKit/Source/web/WebViewImpl.h [modify] https://crrev.com/9427bfd430073a9caa0770ab6653b040520a8aab/third_party/WebKit/Source/web/tests/BrowserControlsTest.cpp
,
Feb 2 2017
benhenry@, reverting my patch (r446693) showed no improvement. Relanding(447155) likewise showed no new regression. The bisects seem convinced it was the original patch though. Any thoughts on how to proceed?
,
Feb 2 2017
+juan - what do you think we should do?
,
Feb 3 2017
Yeah, it's strange. But the regressions are relatively small (<165 KiB) and on a single page, so I'll say it's safe to WontFix; unless we see this coming up again on bisects from other alerts.
,
Mar 14 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by benhenry@google.com
, Jan 10 2017