Oilpan: Count # of collected Persistent handles and use it to estimate the live object size

Oilpan: Count # of collected Persistent handles and use it to estimate live object size

Currently the estimation of the live object size is broken in many ways. This CL makes the estimation more reasonable.

The basic idea is as follows:

1) We keep track of the number of allocated and deallocated persistent handles.

2) When completeSweep() is finished, we estimate the heap size per existing persistent handle. The estimated value is recorded in |heapSizePerPersistent|. Also, we record |allocatedObjectSize| + |markedObjectSize| + |PartitionAlloc::commitedSize| into |liveObjectSizeAtLastSweep|.

3) When scheduleGCIfNeeded() is called, we estimate the live object size with the following formula:

  Estimated live object size = |liveObjectSizeAtLastSweep| - (# of persistent handles that have deallocated since last collectGarbage()) * |heapSizePerPersistent|

BUG=474470
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198821

[haraken, 2015-06-25 02:42:21]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, yutak@chromium.org

[haraken, 2015-06-25 02:46:48]

I begin to think that it would make more sense to count # of collected
persistent handles than # of collected wrappers.

According to yutak's experiment, it seems that there is a long persistent chain
that cannot be collected in one GC cycle. It might be a cause of the peak memory
increase, because we're currently assuming that one GC will collect all objects
and thus the second or third GC won't be triggered.

To avoid that scenario from happening, we can count # of collected persistent
handles, estimate the heap size retained by one persistent handle, and use it in
the GC heuristics.

At the very least, it would be worth visualizing # of collected persistent
handles in about:tracing. It will give us some insights.

[haraken, 2015-07-09 07:27:10]

yutak@: keishi@ and I realized that it's hard to tune precise GCs at page
navigations without fixing the incorrect calculation of the estimated live
object size. So I'm planning to fix the incorrect calculation first in this CL.

Would you take a look at the patch set 4 and experiment if the patch set 4 is
helpful to make the estimation better?

[haraken, 2015-07-09 07:47:18]

As far as I run a couple of benchmarks where the GC frequency matters, I cannot
observe any performance regression. I'll run a full set of blink_perf benchmarks
tonight and am planning to land this CL if the result is fine.

yutak@: So PTAL.

[haraken, 2015-07-10 04:35:06]

On 2015/07/09 07:47:18, haraken wrote:
> As far as I run a couple of benchmarks where the GC frequency matters, I
cannot
> observe any performance regression. I'll run a full set of blink_perf
benchmarks
> tonight and am planning to land this CL if the result is fine.
> 
> yutak@: So PTAL.

I measured blink_perf performance here:
https://docs.google.com/spreadsheets/d/1qFZ5gca9okdtI1ndhw-v-tbU6sU9gmjY35fIl...

It seems the results are too flaky to conclude something. I need to give another
shot tonight. As far as I test the performance of GC-sensitive benchmarks, I
observe no regression though.

[Yuta Kitamura, 2015-07-13 06:38:03]

I'm generally OK with this patch, but read the last
comment especially.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Bl...
File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Bl...
Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:37:
objectsDump->AddScalar("estimated_live_object_size", "bytes",
Heap::liveObjectSizeAtLastSweep());
Not sure if we can change the label here.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1930: s_heapSizePerPersistent = 1024 * 1024;
Isn't this too high?

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:2391: TRACE_COUNTER1("blink_gc",
"Heap::estimatedLiveObjectSizeKB", std::min(Heap::liveObjectSizeAtLastSweep() /
1024, static_cast<size_t>(INT_MAX)));
Please change the label, too.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:562: if (Heap::liveObjectSizeAtLastSweep()
> Heap::heapSizePerPersistent() * Heap::collectedPersistentCount())
nit: I would preserve the return values of Heap::***() functions
to a local variable to avoid calling atomic load twice for each
value.

It's extremely unlikely but may be possible that a race condition
causes a negative overflow if these values are updated between two
atomic loads.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1128:
Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());
I'm starting to wonder why this uses the *current* memory usage
instead of the memory gained through the last sweeping.

Suppose we've finished sweeping for the N-th marking. We could
rewrite this line to something like:

    Heap::setHeapSizePerPersistent(
        <total currentObjectSize() gain in the N-th sweep> /
        <# of persistents freed between the (N-1)-th marking and the N-th
marking>);

The idea is to use the actual memory gain instead of assuming
the memory is owned uniformly among persistents.

This doesn't block your change from landing. Your original
assumption is probably not too bad.

[Yuta Kitamura, 2015-07-13 06:47:16]

Also: your change description seems to deviate from the
actual patch. heapSizePerPersistent is recorded after
sweeping.

And it also has two 2s ;)

[sof, 2015-07-13 07:07:45]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-13 07:07:46]

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:215: state->persistentAllocated();
What about cross-thread persistents?

[haraken, 2015-07-13 08:46:25]

Thanks for the careful review!

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Bl...
File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Bl...
Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:37:
objectsDump->AddScalar("estimated_live_object_size", "bytes",
Heap::liveObjectSizeAtLastSweep());
On 2015/07/13 06:38:02, Yuta Kitamura wrote:
> Not sure if we can change the label here.

This is a customized field for Oilpan in memory-infra, so should be OK.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:215: state->persistentAllocated();
On 2015/07/13 07:07:45, sof wrote:
> What about cross-thread persistents?

Good point. One way to support cross-thread persistents would be to count # of
cross-thread persistents by mainThreadState()->persistentAllocated(). However,
given that # of cross-thread persistents is pretty small, it wouldn't be worth
adding the hack.

Added a TODO on ThreadState::persistentAlloated().

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1930: s_heapSizePerPersistent = 1024 * 1024;
On 2015/07/13 06:38:02, Yuta Kitamura wrote:
> Isn't this too high?

Actually I think INT_MAX is OK here.

Initially, the total heap size is very small. So we'll hit the 50% increase (for
a precise & idle GC) or the 400% increase (for a conservative GC) even if we
don't take into account the memory usage explained by the collected persistent
handles. In other words, s_heapSizePerPersistent doesn't make much sense when
the total heap size is small. So I think it is OK to set it to INT_MAX
initially.

I changed it to INT_MAX with a comment.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:2391: TRACE_COUNTER1("blink_gc",
"Heap::estimatedLiveObjectSizeKB", std::min(Heap::liveObjectSizeAtLastSweep() /
1024, static_cast<size_t>(INT_MAX)));
On 2015/07/13 06:38:02, Yuta Kitamura wrote:
> Please change the label, too.

Done.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:562: if (Heap::liveObjectSizeAtLastSweep()
> Heap::heapSizePerPersistent() * Heap::collectedPersistentCount())
On 2015/07/13 06:38:03, Yuta Kitamura wrote:
> nit: I would preserve the return values of Heap::***() functions
> to a local variable to avoid calling atomic load twice for each
> value.
> 
> It's extremely unlikely but may be possible that a race condition
> causes a negative overflow if these values are updated between two
> atomic loads.

Done.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1128:
Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());
On 2015/07/13 06:38:03, Yuta Kitamura wrote:
> I'm starting to wonder why this uses the *current* memory usage
> instead of the memory gained through the last sweeping.
> 
> Suppose we've finished sweeping for the N-th marking. We could
> rewrite this line to something like:
> 
>     Heap::setHeapSizePerPersistent(
>         <total currentObjectSize() gain in the N-th sweep> /
>         <# of persistents freed between the (N-1)-th marking and the N-th
> marking>);
> 
> The idea is to use the actual memory gain instead of assuming
> the memory is owned uniformly among persistents.
> 
> This doesn't block your change from landing. Your original
> assumption is probably not too bad.

Your point makes sense and that's exactly what I was doing in patch set 1.
However, given the complexity and the fact that we cannot accurately measure the
"gain" through the sweeping (*), I switched to a simpler approach.

(*) For example, in textarea-dom.html, a lot text nodes are created during the
sweeping phase. This will confuse the gain measurement.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1128:
Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());

> Also: your change description seems to deviate from the actual patch.
heapSizePerPersistent is recorded after sweeping.

Updated the CL description. Also I updated the code from:

  Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());

to:

  Heap::setHeapSizePerPersistent(currentObjectSize() /
Heap::persistentCountAtLastGC());

Given that Heap::collectedPersistentCount() has been counted up since the last
collectGarbage(), we must estimate the heapSizePerPersistent based on the
persistent count at the last collectGarabge().

[haraken, 2015-07-13 08:54:58]

BTW, the V8 team is going to make a change where derefObject()s don't get called
during major GCs (i.e., derefObjects() are made incremental and deferred to
later). So it is important to remove the assumption that major GCs will drop a
lot of persistent handles from Oilpan.

[Yuta Kitamura, 2015-07-13 09:03:49]

LGTM

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1930: s_heapSizePerPersistent = 1024 * 1024;
On 2015/07/13 08:46:25, haraken wrote:
> I changed it to INT_MAX with a comment.

INT_MAX may cause overflow on 32-bit arch (because it will
be multiplied by # of freed persistents), so it's probably
better to stay at 1M then.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1128:
Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());
On 2015/07/13 08:46:25, haraken wrote:
> On 2015/07/13 06:38:03, Yuta Kitamura wrote:
> > I'm starting to wonder why this uses the *current* memory usage
> > instead of the memory gained through the last sweeping.
> > 
> > Suppose we've finished sweeping for the N-th marking. We could
> > rewrite this line to something like:
> > 
> >     Heap::setHeapSizePerPersistent(
> >         <total currentObjectSize() gain in the N-th sweep> /
> >         <# of persistents freed between the (N-1)-th marking and the N-th
> > marking>);
> > 
> > The idea is to use the actual memory gain instead of assuming
> > the memory is owned uniformly among persistents.
> > 
> > This doesn't block your change from landing. Your original
> > assumption is probably not too bad.
> 
> Your point makes sense and that's exactly what I was doing in patch set 1.
> However, given the complexity and the fact that we cannot accurately measure
the
> "gain" through the sweeping (*), I switched to a simpler approach.
> 
> (*) For example, in textarea-dom.html, a lot text nodes are created during the
> sweeping phase. This will confuse the gain measurement.

IMO, you can measure the gain by recording the memory size before
and after each sub sweeping task (lazySweep, lazySweepWithDeadline,
and sweepUnsweptPages for example).

[haraken, 2015-07-13 09:07:38]

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1930: s_heapSizePerPersistent = 1024 * 1024;
On 2015/07/13 09:03:49, Yuta Kitamura wrote:
> On 2015/07/13 08:46:25, haraken wrote:
> > I changed it to INT_MAX with a comment.
> 
> INT_MAX may cause overflow on 32-bit arch (because it will
> be multiplied by # of freed persistents), so it's probably
> better to stay at 1M then.

Makes sense; will fix.

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1211573006/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:1128:
Heap::setHeapSizePerPersistent(currentObjectSize() / Heap::persistentCount());
On 2015/07/13 09:03:49, Yuta Kitamura wrote:
> On 2015/07/13 08:46:25, haraken wrote:
> > On 2015/07/13 06:38:03, Yuta Kitamura wrote:
> > > I'm starting to wonder why this uses the *current* memory usage
> > > instead of the memory gained through the last sweeping.
> > > 
> > > Suppose we've finished sweeping for the N-th marking. We could
> > > rewrite this line to something like:
> > > 
> > >     Heap::setHeapSizePerPersistent(
> > >         <total currentObjectSize() gain in the N-th sweep> /
> > >         <# of persistents freed between the (N-1)-th marking and the N-th
> > > marking>);
> > > 
> > > The idea is to use the actual memory gain instead of assuming
> > > the memory is owned uniformly among persistents.
> > > 
> > > This doesn't block your change from landing. Your original
> > > assumption is probably not too bad.
> > 
> > Your point makes sense and that's exactly what I was doing in patch set 1.
> > However, given the complexity and the fact that we cannot accurately measure
> the
> > "gain" through the sweeping (*), I switched to a simpler approach.
> > 
> > (*) For example, in textarea-dom.html, a lot text nodes are created during
the
> > sweeping phase. This will confuse the gain measurement.
> 
> IMO, you can measure the gain by recording the memory size before
> and after each sub sweeping task (lazySweep, lazySweepWithDeadline,
> and sweepUnsweptPages for example).

Yeah, we can do that. That said, it is not yet accurate in a sense that it
doesn't take into account the persistent handles collected by V8 GCs.

At the moment, let me land this CL (after confirming blink_perf numbers). We can
adjust the heuristics in follow-up CLs.

[haraken, 2015-07-13 23:28:59]

No substantial regression or improvement is observed in blink_perf (I manually
verified that >5% regressing benchmarks are just flakes). Landing.

https://docs.google.com/spreadsheets/d/1qFZ5gca9okdtI1ndhw-v-tbU6sU9gmjY35fIl...

[haraken, 2015-07-13 23:29:04]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-07-13 23:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1211573006/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-07-13 23:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211573006/140001

[commit-bot: I haz the power, 2015-07-14 00:58:12]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198821