Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1252683003:
Oilpan: Schedule a precise GC when a page navigates (Closed)
|
Created:
5 years, 5 months ago by keishi Modified:
5 years, 4 months ago CC:
blink-reviews, tyoshino+watch_chromium.org, oilpan-reviews, Mads Ager (chromium), eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin, kouhei+heap_chromium.org, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Schedule a precise GC when a page navigates
Page navigation will likely produce many garbage Nodes so this CL adds a special navigation GC.
Document keeps track of number of attached nodes in Docuement::nodeCount(). When a navigation occurs we calculate the estimatedRemovalRatio from the (number of garbage nodes) / (total node instance count).
The number of garbage nodes is estimated by subtracting the nodeCount of non navigating documents from the total node instance count.
BUG=None
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200467
Patch Set 1 #Patch Set 2 : WIP #
Total comments: 4
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : Added initialization #Patch Set 10 : #
Messages
Total messages: 36 (15 generated)
keishi@chromium.org changed reviewers: + oilpan-reviews@chromium.org
haraken@chromium.org changed reviewers: + haraken@chromium.org
The approach is looking good. https://codereview.chromium.org/1252683003/diff/20001/Source/core/loader/Fram... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1252683003/diff/20001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1026: bool FrameLoader::prepareForCommit() Just a question: When will prepareForCommit() be called? Is it before WindowProxy::closeForNavigation() or after WindowProxy::closeForNavigation()? https://codereview.chromium.org/1252683003/diff/20001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1035: nodeCount += m_frame->domWindow()->document()->nodeCount(); If things happen in the order of: 1) User script removes all nodes from a document. 2) The document navigates away. then the node count reported here would be underestimated. I think you were saying that this is happening in Speedometer. I guess what we really want to count here is (the number of nodes attached to the document) + (the number of nodes that are not attached to any document). Maybe we can count the number by hooking the following functions. - Node's constructor -- A - Node's destructor -- B - Node::insertedInto -- C(i) - Node::removedFrom -- D(i) Then: (the number of nodes attached to the document) + (the number of nodes that are not attached to any document x) = (A - B - \sum (C(i) - D(i))) + (C(x) - D(x)) https://codereview.chromium.org/1252683003/diff/20001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1038: if (ratio > 1) When can this happen? https://codereview.chromium.org/1252683003/diff/20001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1252683003/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:332: bool shouldSchedulePreciseGC(); Remove this.
https://codereview.chromium.org/1252683003/diff/40001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1252683003/diff/40001/Source/core/dom/Documen... Source/core/dom/Document.h:1041: void decrementNodeCount() { m_nodeCount--; } I'd use 'signed int' for m_nodeCount and add ASSERT(m_nodeCount > 0) here. https://codereview.chromium.org/1252683003/diff/40001/Source/core/loader/Fram... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1252683003/diff/40001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1040: ThreadState::current()->schedulePreciseGC(); Shall we change the signature to schedulePageNavigationGCIfNeeded(ratio) (which calls schedulePreciseGC if the ratio meets the predicate)? https://codereview.chromium.org/1252683003/diff/40001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/40001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:926: Heap::collectGarbage(NoHeapPointersOnStack, GCWithSweep, Heap::PreciseGC); Maybe is the regression you're observing in the shadow-dom benchmarks related to the synchronous sweep? What happens if we change this to GCWithoutSweep? (Maybe the regression in the shadow-dom benchmarks will be gone but the regression in Speedometer re-appears.)
noderatio2 is patch set 4. blink_perf numbers look mostly the same as ToT oilpan. I'm not sure why shadow-dom benchmarks were bad last time. https://257e68df4dae930120a4c67b6f7a507d5042fbc3.googledrive.com/host/0B4aiM9... I've did some browsing while printing estimatedLiveObjectSize on navigatio gc and markedObjectSize in postSweep and I think it looks good. Navigation GC seems to be happening between every speedometer test and the estimations are not too small most of the time. https://codereview.chromium.org/1252683003/diff/40001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1252683003/diff/40001/Source/core/dom/Documen... Source/core/dom/Document.h:1041: void decrementNodeCount() { m_nodeCount--; } On 2015/08/03 01:03:59, haraken wrote: > > I'd use 'signed int' for m_nodeCount and add ASSERT(m_nodeCount > 0) here. Done. https://codereview.chromium.org/1252683003/diff/40001/Source/core/loader/Fram... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1252683003/diff/40001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1040: ThreadState::current()->schedulePreciseGC(); On 2015/08/03 01:03:59, haraken wrote: > > Shall we change the signature to schedulePageNavigationGCIfNeeded(ratio) (which > calls schedulePreciseGC if the ratio meets the predicate)? Done.
Add your performance results to the CL description. https://codereview.chromium.org/1252683003/diff/60001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1252683003/diff/60001/Source/core/dom/Documen... Source/core/dom/Document.h:1047: int nodeCount() { return m_nodeCount; } Add const. https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:654: { Add: if (UNLIKELY(isGCForbidden())) return false; if (shouldForceMemoryPressureGC()) return true; https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:669: } bool ThreadState::shouldSchedulePageNavigationGC(float estimatedRemovalRatio) { ...; } void ThreadState::schedulePageNavigationGCIfNeeded(float estimatedRemovalRatio) { ASSERT(checkThread()); // Finish on-going lazy sweeping. completeSweep(); <--- I think it makes sense to do this. ASSERT(!isSweepingInProgress()); ASSERT(!sweepForbidden()); Heap::reportMemoryUsageForTracing(); if (shouldSchedulePageNavigationGC(estimatedRemovalRatio)) schedulePageNavigationGC(); } void ThreadState::schedulePageNavigationGC() { ASSERT(checkThread()); ASSERT(!isSweepingInProgress()); setGCState(PageNavigationGCScheduled); } https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:929: Heap::collectGarbage(NoHeapPointersOnStack, GCWithSweep, Heap::PreciseGC); Hmm, I'm fine with forcing sweeping for navigation GCs but want to avoid forcing sweeping for non-navigation GCs. Can we introduce a new state "PageNavigationGCScheduled"?
Here is the telemetry results for this CL. https://257e68df4dae930120a4c67b6f7a507d5042fbc3.googledrive.com/host/0B4aiM9... I think this CL (noderatio22) doesn't have any significant regressions compared to "oilpan". Some blink_perf shadow_dom test results are dropping in this run but I can't reproduce the regression at all when running the tests manually. So performance wise I think we can land this CL. https://codereview.chromium.org/1252683003/diff/60001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1252683003/diff/60001/Source/core/dom/Documen... Source/core/dom/Document.h:1047: int nodeCount() { return m_nodeCount; } On 2015/08/06 07:52:42, haraken wrote: > > Add const. Done. https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:654: { On 2015/08/06 07:52:43, haraken wrote: > > Add: > > if (UNLIKELY(isGCForbidden())) > return false; > > if (shouldForceMemoryPressureGC()) > return true; Done. https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:669: } On 2015/08/06 07:52:42, haraken wrote: > > bool ThreadState::shouldSchedulePageNavigationGC(float estimatedRemovalRatio) > { > ...; > } > > void ThreadState::schedulePageNavigationGCIfNeeded(float estimatedRemovalRatio) > { > ASSERT(checkThread()); > // Finish on-going lazy sweeping. > completeSweep(); <--- I think it makes sense to do this. > ASSERT(!isSweepingInProgress()); > ASSERT(!sweepForbidden()); > > Heap::reportMemoryUsageForTracing(); > if (shouldSchedulePageNavigationGC(estimatedRemovalRatio)) > schedulePageNavigationGC(); > } > > void ThreadState::schedulePageNavigationGC() > { > ASSERT(checkThread()); > ASSERT(!isSweepingInProgress()); > setGCState(PageNavigationGCScheduled); > } Done. https://codereview.chromium.org/1252683003/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:929: Heap::collectGarbage(NoHeapPointersOnStack, GCWithSweep, Heap::PreciseGC); On 2015/08/06 07:52:43, haraken wrote: > > Hmm, I'm fine with forcing sweeping for navigation GCs but want to avoid forcing > sweeping for non-navigation GCs. Can we introduce a new state > "PageNavigationGCScheduled"? > Done.
haraken@chromium.org changed reviewers: + sigbjornf@opera.se
LGTM % the state transition in setGCState is correctly fixed. Sigbjorn: Would you mind taking another look at ThreadState.cpp when you have a cycle? https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element.h File Source/core/dom/Element.h (right): https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element... Source/core/dom/Element.h:808: insertionPoint->document().topDocument().incrementNodeCount(); You can move this code into the above 'insertionPoint->inDocument()' clause. https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element... Source/core/dom/Element.h:822: insertionPoint->document().topDocument().decrementNodeCount(); Ditto. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:679: // Finish on-going lazy sweeping. Add: // TODO(haraken): It might not make sense to force completeSweep() for all page navigations. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:188: PageNavigationGCScheduled, You need to add PageNavigationGCScheduled to ThreadState::setGCState. I think the transition should be very similar to PreciseGCScheduled. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:335: bool shouldSchedulePageNavigationGC(float estimatedRemovalRatio); Move this declaration to the other shouldScheduleXXX methods.
https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element.h File Source/core/dom/Element.h (right): https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element... Source/core/dom/Element.h:808: insertionPoint->document().topDocument().incrementNodeCount(); On 2015/08/11 07:43:08, haraken wrote: > > You can move this code into the above 'insertionPoint->inDocument()' clause. Done. https://codereview.chromium.org/1252683003/diff/80001/Source/core/dom/Element... Source/core/dom/Element.h:822: insertionPoint->document().topDocument().decrementNodeCount(); On 2015/08/11 07:43:08, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:679: // Finish on-going lazy sweeping. On 2015/08/11 07:43:08, haraken wrote: > > Add: > > // TODO(haraken): It might not make sense to force completeSweep() for all > page navigations. Done. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:188: PageNavigationGCScheduled, On 2015/08/11 07:43:08, haraken wrote: > > You need to add PageNavigationGCScheduled to ThreadState::setGCState. I think > the transition should be very similar to PreciseGCScheduled. > Please check the change to ThreadState::setGCState to see if I got this right. PageNavigationGCScheduled can be set while NoGCScheduled or any of theGCScheduled states and I didn't do completeSweep because I already do it in ThreadState::schedulePageNavigationGCIfNeeded. https://codereview.chromium.org/1252683003/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:335: bool shouldSchedulePageNavigationGC(float estimatedRemovalRatio); On 2015/08/11 07:43:08, haraken wrote: > > Move this declaration to the other shouldScheduleXXX methods. Done.
Still LGTM. It looks like sigbjorn is near-ooo, so let's proceed with landing this one. https://codereview.chromium.org/1252683003/diff/100001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/100001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:890: case PageNavigationGCScheduled: I'd slightly prefer including this into the above XXXGCScheduled list. It is ok to call completeSweep() multiple times. The second and later completeSweep()s are just ignored.
https://codereview.chromium.org/1252683003/diff/100001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1252683003/diff/100001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:890: case PageNavigationGCScheduled: On 2015/08/11 08:30:20, haraken wrote: > > I'd slightly prefer including this into the above XXXGCScheduled list. It is ok > to call completeSweep() multiple times. The second and later completeSweep()s > are just ignored. Thanks! Done. Proceeding to commit.
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 Link to the patchset: https://codereview.chromium.org/1252683003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252683003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252683003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/73265)
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 Link to the patchset: https://codereview.chromium.org/1252683003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252683003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252683003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/73293)
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 Link to the patchset: https://codereview.chromium.org/1252683003/#ps160001 (title: "Added initialization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252683003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252683003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
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 Link to the patchset: https://codereview.chromium.org/1252683003/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1252683003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1252683003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200467
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
If a Node moves from document A to document B, how is that accounted for?
Message was sent while issue was closed.
On 2015/08/22 06:13:26, sof wrote: > If a Node moves from document A to document B, how is that accounted for? Then removedFrom() is called for A and insertedTo() is called for B, isn't it?
Message was sent while issue was closed.
On 2015/08/22 08:01:10, haraken wrote: > On 2015/08/22 06:13:26, sof wrote: > > If a Node moves from document A to document B, how is that accounted for? > > Then removedFrom() is called for A and insertedTo() is called for B, isn't it? Yes, it is - I can see the code path now. Just wondered if there was a fast path that didn't, which could have helped explain the counting unsync. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
