Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1698703002:
Disable page navigation GC (Closed)
|
Created:
4 years, 10 months ago by keishi Modified:
4 years, 10 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, oilpan-reviews, Mads Ager (chromium), loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, kinuko+watch, Nate Chapin, kouhei+heap_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable page navigation GC
Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact.
BUG=588029
Committed: https://crrev.com/6ee2be06467613d8770c041a30e123cc23d8e752
Cr-Commit-Position: refs/heads/master@{#376385}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Remove PageNavigationGC #Patch Set 4 : Temporarily disable PageNavigationGC #
Total comments: 2
Patch Set 5 : added bug no #Messages
Total messages: 29 (10 generated)
Description was changed from ========== Adjust page navigation GC threshold to decrease occurance We originally tried to estimate removal rate by counting the (nodes of the detaching document) + (already deatached nodes). Page navigation threshold was scheduled even when we were navigating from about:blank because (already deatached nodes) count was inflated. Nodes detached from JS is already accounted for in estimatedLiveSize through collectedWrapperCount so we didn't need this. BUG= ========== to ========== Adjust page navigation GC threshold to decrease occurance We originally tried to estimate removal rate by counting the (nodes of the detaching document) + (already deatached nodes). Page navigation threshold was scheduled even when we were navigating from about:blank because (already deatached nodes) count was inflated. Nodes detached from JS is already accounted for in estimatedLiveSize through collectedWrapperCount so we didn't need this. BUG=None ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org
While investigating sfgate.com I noticed that PageNavigationGC was invoked too much. The problems were: - PageNavigationGC was called around 10 times for one load of sfgate.com, probably because of the iframes. - PageNavigationGC was happening even when navigating from about:blank When I load sfgate.com in a new content_shell, nodeCount would be around 250 even when navigating from about:blank. This CL - fixes the nodeCount calculation - introduces a estimatedRemovalRatioThreashold because if its close to zero we should let IdleGC handle it to avoid jank. - increases totalMemorySizeThreshold because I think it should not be the same as IdleGC and more like V8FollowupGC. I've tried this CL on a trybot and speedometer doesn't seem to degrade. http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02...
Thanks for digging into this! https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:567: if (estimatedRemovalRatio == 0.01) I'm confused with this branch. - Are you intending 'estimatedRemovalRatio < 0.01'? - I think estimatedRemovalRatio should be renamed to estimatedLiveNodesRatio. If so, 'estimatedLiveNodesRatio < 0.01' means that "don't schedule a page navigation GC if # of live nodes is small". This doesn't make sense. - 'return false' won't schedule an idle GC.
https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:567: if (estimatedRemovalRatio == 0.01) On 2016/02/14 23:23:04, haraken wrote: > > I'm confused with this branch. > > - Are you intending 'estimatedRemovalRatio < 0.01'? Yes, sorry. > - I think estimatedRemovalRatio should be renamed to estimatedLiveNodesRatio. If > so, 'estimatedLiveNodesRatio < 0.01' means that "don't schedule a page > navigation GC if # of live nodes is small". This doesn't make sense. FrameLoader is calculating (# of nodes in detaching document) / (# of nodes) so this isn't a live node ratio. My thinking was that PageNavigationGC has penalties like impact on PLT and jank so we should let normal IdleGC logic handle most of the time. If the estimatedRemovalRatio is low this isn't really a PageNavigationGC, more like a IdleGC with the extra penalties. I'll fix the estimatedRemovalRatio and measure some more.
On 2016/02/15 01:40:50, keishi wrote: > https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1698703002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:567: if > (estimatedRemovalRatio == 0.01) > On 2016/02/14 23:23:04, haraken wrote: > > > > I'm confused with this branch. > > > > - Are you intending 'estimatedRemovalRatio < 0.01'? > Yes, sorry. > > - I think estimatedRemovalRatio should be renamed to estimatedLiveNodesRatio. > If > > so, 'estimatedLiveNodesRatio < 0.01' means that "don't schedule a page > > navigation GC if # of live nodes is small". This doesn't make sense. > FrameLoader is calculating (# of nodes in detaching document) / (# of nodes) so > this isn't a live node ratio. > > My thinking was that PageNavigationGC has penalties like impact on PLT and jank > so we should let normal IdleGC logic handle most of the time. If the > estimatedRemovalRatio is low this isn't really a PageNavigationGC, more like a > IdleGC with the extra penalties. Thanks, this makes sense to me.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
What happens if you remove page navigation GCs altogether on something like speedometer (and others which motivated its intro)? Have other changes rendered it not useful any longer?
Description was changed from ========== Adjust page navigation GC threshold to decrease occurance We originally tried to estimate removal rate by counting the (nodes of the detaching document) + (already deatached nodes). Page navigation threshold was scheduled even when we were navigating from about:blank because (already deatached nodes) count was inflated. Nodes detached from JS is already accounted for in estimatedLiveSize through collectedWrapperCount so we didn't need this. BUG=None ========== to ========== Remove page navigation GC Page navigation GC is firing too much. Removing it doesn't seem to effect speedometer performance so it isn't necessary any more. BUG=None ==========
Description was changed from ========== Remove page navigation GC Page navigation GC is firing too much. Removing it doesn't seem to effect speedometer performance so it isn't necessary any more. BUG=None ========== to ========== Remove page navigation GC Page navigation GC is firing too much. Removing it doesn't seem to effect speedometer performance. BUG=None ==========
On 2016/02/15 06:21:02, sof wrote: > What happens if you remove page navigation GCs altogether on something like > speedometer (and others which motivated its intro)? Have other changes rendered > it not useful any longer? You're right. I ran the try bots on speedometer and blink_perf.parser and it seems like it is not longer needed. bot test result mac_retina_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_10_11_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_10_10_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_s5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus9_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus6_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus7_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... linux_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... linux_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_10_11_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_retina_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_hdd_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus6_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus5_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus7_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus9_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_s5_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... linux_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_retina_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_hdd_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_10_11_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... mac_10_10_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_s5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus7_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus9_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02...
On 2016/02/15 11:39:56, keishi wrote: > On 2016/02/15 06:21:02, sof wrote: > > What happens if you remove page navigation GCs altogether on something like > > speedometer (and others which motivated its intro)? Have other changes > rendered > > it not useful any longer? > > You're right. I ran the try bots on speedometer and blink_perf.parser and it > seems like it is not longer needed. > > bot test result > > mac_retina_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_10_11_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_10_10_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_s5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus9_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus6_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus7_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > linux_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > linux_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_10_11_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_retina_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_hdd_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus6_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus5_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus7_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus9_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_s5_perf_bisect blink_perf.parser http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > linux_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_retina_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_hdd_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_10_11_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > mac_10_10_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_s5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus7_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus9_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus5_perf_bisect speedometer http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... What about the peak memory increase of memory.top_7_stress? I remember that was another motivation of the navigation GC. (I'm fine with just removing the navigation GC if it doesn't regress performance/memory.)
The test only completed on a few bots. Looks good except android_nexus5_perf_bisect. Some measurements on android_nexus5_perf_bisect is bad but blink_gc is good so I'm hoping its not related. winx64ati_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... win_x64_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... winx64intel_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus7_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus9_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus6_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... android_nexus5_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02...
On 2016/02/16 02:36:40, keishi wrote: > The test only completed on a few bots. Looks good except > android_nexus5_perf_bisect. Some measurements on android_nexus5_perf_bisect is > bad but blink_gc is good so I'm hoping its not related. > > winx64ati_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > win_x64_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > winx64intel_perf_bisect memory.top_7_stress http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus7_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus9_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus6_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... > android_nexus5_perf_bisect memory.memory_health_plan http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-02... Thanks for the investigation! LGTM to removing page navigation GCs for now. That said, I think Oilpan should take into account page navigations in some sense. The fact that a page that contains a big document has navigated away is an important signal that implies a lot of garbage (just like the fact that a V8 major GC has happened). I think Oilpan should use the signal as a hint to decide the next GC timing.
It is important to fix the long-time pause in sfgate.com as soon as possible. So I'd propose: 1) Land a CL that just disables page navigation GCs (without removing code for navigation GCs). And see how it goes. 2) Investigate a bit more about the navigation GCs. If it's not really needed, we can just remove the code. You're saying that navigation GCs are fired too much in sfgate.com, but how much is the collection rate of the navigation GCs? How much is the heap size when the navigation GCs are triggered? If the collection rate is high and the heap size is large, the navigation GCs may be doing a right thing. (Either way I think we can investigate it after landing 1).)
On 2016/02/17 10:37:50, haraken wrote: > It is important to fix the long-time pause in http://sfgate.com as soon as possible. So > I'd propose: > > 1) Land a CL that just disables page navigation GCs (without removing code for > navigation GCs). And see how it goes. > > 2) Investigate a bit more about the navigation GCs. If it's not really needed, > we can just remove the code. > > You're saying that navigation GCs are fired too much in http://sfgate.com, but how much > is the collection rate of the navigation GCs? How much is the heap size when the > navigation GCs are triggered? If the collection rate is high and the heap size > is large, the navigation GCs may be doing a right thing. (Either way I think we > can investigate it after landing 1).) I've recorded a wpr for sfgate.com and used telemetry to collect traces but I found that the types and number of GCs are different for every run and very unpredictable. So it isn't for all runs, but frequently loading sfgate.com will have around 10 PageNavigationGCs. Most of them with less than 10% collection rate, and triggered by the estimatedRemovalRate issue. I can't conclude what the side effects of removing PageNavigationGC will be so, as you suggested, I will temporarily disable PageNavigationGC and see how it goes.
On 2016/02/18 16:18:59, keishi wrote: > On 2016/02/17 10:37:50, haraken wrote: > > It is important to fix the long-time pause in http://sfgate.com as soon as > possible. So > > I'd propose: > > > > 1) Land a CL that just disables page navigation GCs (without removing code for > > navigation GCs). And see how it goes. > > > > 2) Investigate a bit more about the navigation GCs. If it's not really needed, > > we can just remove the code. > > > > You're saying that navigation GCs are fired too much in http://sfgate.com, but > how much > > is the collection rate of the navigation GCs? How much is the heap size when > the > > navigation GCs are triggered? If the collection rate is high and the heap size > > is large, the navigation GCs may be doing a right thing. (Either way I think > we > > can investigate it after landing 1).) > > I've recorded a wpr for http://sfgate.com and used telemetry to collect traces but I > found that the types and number of GCs are different for every run and very > unpredictable. > So it isn't for all runs, but frequently loading http://sfgate.com will have around 10 > PageNavigationGCs. Most of them with less than 10% collection rate, and > triggered by the estimatedRemovalRate issue. > > I can't conclude what the side effects of removing PageNavigationGC will be so, > as you suggested, I will temporarily disable PageNavigationGC and see how it > goes. Thanks a lot for the investigations. Just disabling navigation GCs sounds like a good way to go.
lgtm https://codereview.chromium.org/1698703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1698703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:567: // TODO(keishi): Temporarily disabled to observe impact. Could you create a bug for tracking this issue, and refer to it here & via a BUG= ?
LGTM
Description was changed from ========== Remove page navigation GC Page navigation GC is firing too much. Removing it doesn't seem to effect speedometer performance. BUG=None ========== to ========== Disable page navigation GC Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact. BUG=588029 ==========
https://codereview.chromium.org/1698703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1698703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:567: // TODO(keishi): Temporarily disabled to observe impact. On 2016/02/18 18:33:37, sof wrote: > Could you create a bug for tracking this issue, and refer to it here & via a > BUG= ? Done.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1698703002/#ps80001 (title: "added bug no")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698703002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698703002/80001
Message was sent while issue was closed.
Description was changed from ========== Disable page navigation GC Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact. BUG=588029 ========== to ========== Disable page navigation GC Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact. BUG=588029 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Disable page navigation GC Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact. BUG=588029 ========== to ========== Disable page navigation GC Page navigation GC is firing too much in some circumstances. This CL disables it to observe impact. BUG=588029 Committed: https://crrev.com/6ee2be06467613d8770c041a30e123cc23d8e752 Cr-Commit-Position: refs/heads/master@{#376385} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ee2be06467613d8770c041a30e123cc23d8e752 Cr-Commit-Position: refs/heads/master@{#376385}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1750993002/ by keishi@chromium.org. The reason for reverting is: Caused some unintended perf regressions.. |
