Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
850663002:
Oilpan: fix computation of ThreadState::m_collectionRate. (Closed)
DescriptionOilpan: fix computation of ThreadState::m_collectionRate.
Sample allocated memory/object size before initiating a GC.
R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188378
Patch Set 1 #
Total comments: 6
Patch Set 2 : rename to m_allocatedObjectSizeBeforeGC #Patch Set 3 : assert for valid m_collectionRate #Patch Set 4 : compute m_collectionRate in postGC() #
Total comments: 3
Patch Set 5 : Undo ill-effects of previous patchset #Messages
Total messages: 18 (3 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize(); Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases probably? https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:897: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize() + Heap::markedObjectSize(); Shall we rename it to m_objectSizeBeforeGC?
https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize(); On 2015/01/13 11:56:48, haraken wrote: > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > probably? Yes; m_collectionRate then mapping to +Inf. https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:897: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize() + Heap::markedObjectSize(); On 2015/01/13 11:56:48, haraken wrote: > > Shall we rename it to m_objectSizeBeforeGC? Done.
https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize(); On 2015/01/13 12:11:53, sof wrote: > On 2015/01/13 11:56:48, haraken wrote: > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > > probably? > > Yes; m_collectionRate then mapping to +Inf. oh... Also let's add a zero-division check to the calculation of m_collectionRate.
https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping = Heap::allocatedObjectSize(); On 2015/01/13 12:23:08, haraken wrote: > On 2015/01/13 12:11:53, sof wrote: > > On 2015/01/13 11:56:48, haraken wrote: > > > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > > > probably? > > > > Yes; m_collectionRate then mapping to +Inf. > > oh... > > Also let's add a zero-division check to the calculation of m_collectionRate. Asserted on m_collectionRate's range.
On 2015/01/13 12:35:57, sof wrote: > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > File Source/platform/heap/ThreadState.cpp (left): > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping = > Heap::allocatedObjectSize(); > On 2015/01/13 12:23:08, haraken wrote: > > On 2015/01/13 12:11:53, sof wrote: > > > On 2015/01/13 11:56:48, haraken wrote: > > > > > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > > > > probably? > > > > > > Yes; m_collectionRate then mapping to +Inf. > > > > oh... > > > > Also let's add a zero-division check to the calculation of m_collectionRate. > > Asserted on m_collectionRate's range. Seeing some > 1.0 rates being computed, which I need to understand.
On 2015/01/13 15:01:42, sof wrote: > On 2015/01/13 12:35:57, sof wrote: > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > File Source/platform/heap/ThreadState.cpp (left): > > > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping > = > > Heap::allocatedObjectSize(); > > On 2015/01/13 12:23:08, haraken wrote: > > > On 2015/01/13 12:11:53, sof wrote: > > > > On 2015/01/13 11:56:48, haraken wrote: > > > > > > > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > > > > > probably? > > > > > > > > Yes; m_collectionRate then mapping to +Inf. > > > > > > oh... > > > > > > Also let's add a zero-division check to the calculation of m_collectionRate. > > > > Asserted on m_collectionRate's range. > > Seeing some > 1.0 rates being computed, which I need to understand. Also we might want to run LargeDistribution, EventDispatchingShadowTrees and EventDispatchingNestedInShadowTrees to see performance. I guess that this change increases the number of conservative GCs (because this change corrected m_collectionRate from +inf to some value in [0,1]) and it might cause some regression. (I'm not intending to say that we shouldn't correct m_collectionRate though.)
On 2015/01/13 15:05:17, haraken wrote: > On 2015/01/13 15:01:42, sof wrote: > > On 2015/01/13 12:35:57, sof wrote: > > > > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > > File Source/platform/heap/ThreadState.cpp (left): > > > > > > > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > > Source/platform/heap/ThreadState.cpp:1041: > m_allocatedObjectSizeBeforeSweeping > > = > > > Heap::allocatedObjectSize(); > > > On 2015/01/13 12:23:08, haraken wrote: > > > > On 2015/01/13 12:11:53, sof wrote: > > > > > On 2015/01/13 11:56:48, haraken wrote: > > > > > > > > > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most > cases > > > > > > probably? > > > > > > > > > > Yes; m_collectionRate then mapping to +Inf. > > > > > > > > oh... > > > > > > > > Also let's add a zero-division check to the calculation of > m_collectionRate. > > > > > > Asserted on m_collectionRate's range. > > > > Seeing some > 1.0 rates being computed, which I need to understand. > > Also we might want to run LargeDistribution, EventDispatchingShadowTrees and > EventDispatchingNestedInShadowTrees to see performance. I guess that this change > increases the number of conservative GCs (because this change corrected > m_collectionRate from +inf to some value in [0,1]) and it might cause some > regression. (I'm not intending to say that we shouldn't correct m_collectionRate > though.) Yes, the m_collectionRate threshold in shouldForceConservativeGC() wouldn't always hold now. These results, https://docs.google.com/spreadsheets/d/1y_y2jzSwJCt9DtP9wwIEInCYUUGwC0jZ683OQ... , shows it doesn't make much of a difference on blink_perf.shadow_dom+events.
On 2015/01/13 15:01:42, sof wrote: > On 2015/01/13 12:35:57, sof wrote: > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > File Source/platform/heap/ThreadState.cpp (left): > > > > > https://codereview.chromium.org/850663002/diff/1/Source/platform/heap/ThreadS... > > Source/platform/heap/ThreadState.cpp:1041: m_allocatedObjectSizeBeforeSweeping > = > > Heap::allocatedObjectSize(); > > On 2015/01/13 12:23:08, haraken wrote: > > > On 2015/01/13 12:11:53, sof wrote: > > > > On 2015/01/13 11:56:48, haraken wrote: > > > > > > > > > > Before this CL, m_allocatedObjectSizeBeforeSweeping was 0 in most cases > > > > > probably? > > > > > > > > Yes; m_collectionRate then mapping to +Inf. > > > > > > oh... > > > > > > Also let's add a zero-division check to the calculation of m_collectionRate. > > > > Asserted on m_collectionRate's range. > > Seeing some > 1.0 rates being computed, which I need to understand. Delaying the computation until the main thread left its safe point & called postGCProcessing() could lead to other threads racing ahead. Made m_collectionRate be computed in postGC() instead; take another look?
https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:897: m_collectionRate = 1.0 * Heap::markedObjectSize() / m_allocatedObjectSizeBeforeGC; Heap::markedObjectSize() is accumulated in the (lazy) sweeping phase, and thus Heap::markedObjectSize() would be 0 at this point in most cases, wouldn't it? Things happen in the order of preGC => marking => postGC => lazy sweeping. (However, if that is the case, m_collectionRate will be evaluated as 0 in most cases and thus shouldForceConservativeGC() will use the not-high-collection-mode condition to trigger conservative GCs. I'm wondering why it doesn't regress performance of LargeDistribution.)
https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:897: m_collectionRate = 1.0 * Heap::markedObjectSize() / m_allocatedObjectSizeBeforeGC; On 2015/01/14 01:36:26, haraken wrote: > > Heap::markedObjectSize() is accumulated in the (lazy) sweeping phase, and thus > Heap::markedObjectSize() would be 0 at this point in most cases, wouldn't it? > Things happen in the order of preGC => marking => postGC => lazy sweeping. > Thanks, I realized the same overnight. Sorry about the pointless CL churn, will address. > (However, if that is the case, m_collectionRate will be evaluated as 0 in most > cases and thus shouldForceConservativeGC() will use the not-high-collection-mode > condition to trigger conservative GCs. I'm wondering why it doesn't regress > performance of LargeDistribution.) The miscalculation above represents the other extreme, in a sense. I'll gather some more data on what&how conservative GCs that triggers, once the rate calculation has been sorted out.
https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:897: m_collectionRate = 1.0 * Heap::markedObjectSize() / m_allocatedObjectSizeBeforeGC; On 2015/01/14 06:14:27, sof wrote: > On 2015/01/14 01:36:26, haraken wrote: > > > > Heap::markedObjectSize() is accumulated in the (lazy) sweeping phase, and thus > > Heap::markedObjectSize() would be 0 at this point in most cases, wouldn't it? > > Things happen in the order of preGC => marking => postGC => lazy sweeping. > > > > Thanks, I realized the same overnight. Sorry about the pointless CL churn, will > address. > > > (However, if that is the case, m_collectionRate will be evaluated as 0 in most > > cases and thus shouldForceConservativeGC() will use the > not-high-collection-mode > > condition to trigger conservative GCs. I'm wondering why it doesn't regress > > performance of LargeDistribution.) > > The miscalculation above represents the other extreme, in a sense. I'll gather > some more data on what&how conservative GCs that triggers, once the rate > calculation has been sorted out. Now tidied, to some extent. The lagging sweeps across threads is a bit confusing to interpret in a sense-making manner. Re: LargeDistributionWithoutLayout -- the conservative GCs that it triggers aren't via the two special cases in shouldForceConservativeGC(), so there's no change in behavior to be expected wrt it and this CL.
On 2015/01/14 09:53:24, sof wrote: > https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... > File Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/850663002/diff/60001/Source/platform/heap/Thr... > Source/platform/heap/ThreadState.cpp:897: m_collectionRate = 1.0 * > Heap::markedObjectSize() / m_allocatedObjectSizeBeforeGC; > On 2015/01/14 06:14:27, sof wrote: > > On 2015/01/14 01:36:26, haraken wrote: > > > > > > Heap::markedObjectSize() is accumulated in the (lazy) sweeping phase, and > thus > > > Heap::markedObjectSize() would be 0 at this point in most cases, wouldn't > it? > > > Things happen in the order of preGC => marking => postGC => lazy sweeping. > > > > > > > Thanks, I realized the same overnight. Sorry about the pointless CL churn, > will > > address. > > > > > (However, if that is the case, m_collectionRate will be evaluated as 0 in > most > > > cases and thus shouldForceConservativeGC() will use the > > not-high-collection-mode > > > condition to trigger conservative GCs. I'm wondering why it doesn't regress > > > performance of LargeDistribution.) > > > > The miscalculation above represents the other extreme, in a sense. I'll gather > > some more data on what&how conservative GCs that triggers, once the rate > > calculation has been sorted out. > > Now tidied, to some extent. The lagging sweeps across threads is a bit confusing > to interpret in a sense-making manner. > > Re: LargeDistributionWithoutLayout -- the conservative GCs that it triggers > aren't via the two special cases in shouldForceConservativeGC(), so there's no > change in behavior to be expected wrt it and this CL. Thanks for the digging. LGTM.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850663002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188378 |
