Oilpan: adjust GC policy under memory pressure.

Oilpan: adjust GC policy under memory pressure.

When notified of a v8 major GC, check if we're under memory pressure and
overdue an Oilpan GC & urgently. If so, force a conservative GC right
away.

This addresses dom-modify.html OOM troubles on win32, in particular. It
runs into the allocation pattern where the Oilpan allocator is able to
handle a tight loop just allocating Text nodes by bump allocating each
such node. Each Text node has a large external string allocation however,
which leads to a steep ramp-upin overall allocation amounts to the point
where it might OOM as no Oilpan GCs will run. v8 will however notice the
increasing memory pressure and schedule major GCs of its own.

Oilpan is notified after each v8 major GC, so use that opportunity to
check for memory pressure & force an Oilpan GC right away.

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196953

[haraken, 2015-06-11 04:10:30]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-06-11 04:10:31]

LGTM

It is sad that we're playing around with heuristics, but given that V8 GC has
been doing a similar thing for years, it seems we cannot avoid this route.

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:575: namespace {

Or static. No strong opinion.

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:577: inline bool
shouldForceMemoryPressureGC(size_t currentSize, size_t estimatedSize)

currentObjectSize, estimatedLiveObjectSize

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:584: return (currentSize >> 10) >
(estimatedSize >> 10) * 3 / 2;

Add a comment about why we want to shift the size.

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:806: size_t currentObjectSize =
Heap::allocatedObjectSize() + Heap::markedObjectSize() +
WTF::Partitions::totalSizeOfCommittedPages();

Or you can do:

  if (shouldForceMemoryPressureGC()) {
    Heap::collectGarbage(HeapPointersOnStack, GCWithoutSweep,
Heap::ConservativeGC);
    return;
  }

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:816:
Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);

I think we should do this first.

Maybe a better fix would be:

1) Rename this function to didV8MajorGC.

2) Change the code to:

  void didV8MajorGC() {
    if (isMainThread()) {
      Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);
      scheduleGCIfNeeded();
    }
  }

3) Remove scheduleIdleGC() and schedulePreciseGC() from majorGCEpilogue.

[sof, 2015-06-11 06:22:17]

On 2015/06/11 04:10:31, haraken wrote:
> LGTM
> 
> It is sad that we're playing around with heuristics, but given that V8 GC has
> been doing a similar thing for years, it seems we cannot avoid this route.
> 

The route being the path to a global memory coordinator? :)

I consider this is Oilpan taking another indirect signal into account for its GC
policy -- v8 detecting memory pressure building & forcing a major GC. We already
do sample PA allocation amounts and keep track of allocations & live object sets
internally.

We don't have to use the mechanism here, it is somewhat brute force, but
expedient. i.e., instead of forcing a conservative GC during the v8 major GC
notification, we could set an urgent GC flag. Which allocateObject() would
consider, and fall into a should-GC check. The downside is that this imposes
more code on the allocation code path.

[haraken, 2015-06-11 08:33:47]

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

https://codereview.chromium.org/1174123002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:810: size_t currentObjectSize =
Heap::allocatedObjectSize() + Heap::markedObjectSize() +
WTF::Partitions::totalSizeOfCommittedPages();

Here can we just call:

  Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);
  scheduleGCIfNeeded();

?

[sof, 2015-06-11 08:45:02]

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:584: return (currentSize >> 10) >
(estimatedSize >> 10) * 3 / 2;
On 2015/06/11 04:10:30, haraken wrote:
> 
> Add a comment about why we want to shift the size.

Done.

https://codereview.chromium.org/1174123002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:816:
Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);
On 2015/06/11 04:10:31, haraken wrote:
> 
> I think we should do this first.
> 
> Maybe a better fix would be:
> 
> 1) Rename this function to didV8MajorGC.
> 
> 2) Change the code to:
> 
>   void didV8MajorGC() {
>     if (isMainThread()) {
>       Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);
>       scheduleGCIfNeeded();
>     }
>   }
> 
> 3) Remove scheduleIdleGC() and schedulePreciseGC() from majorGCEpilogue.

Nice. I think that makes good sense, shifting this code over to the Oilpan side.
Done.

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

https://codereview.chromium.org/1174123002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:810: size_t currentObjectSize =
Heap::allocatedObjectSize() + Heap::markedObjectSize() +
WTF::Partitions::totalSizeOfCommittedPages();
On 2015/06/11 08:33:47, haraken wrote:
> 
> Here can we just call:
> 
>   Heap::setEstimatedLiveObjectSize(Heap::estimatedLiveObjectSize() / 2);
>   scheduleGCIfNeeded();
> 
> ?

Sure, bad merge including old code while moving this CL between local branches;
please disregard ps#2.

[sof, 2015-06-11 08:48:20]

https://codereview.chromium.org/1174123002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1174123002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8GCController.cpp:412: void
V8GCController::majorGCEpilogue(v8::Isolate* isolate)
nit: having these separate minor/major methods do not add too much value now,
but i left it alone.

[haraken, 2015-06-11 08:49:17]

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

https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:837: completeSweep();

Can we move the completeSweep() into shouldForceMemoryPressureGC()?

[sof, 2015-06-11 08:56:14]

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

https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:837: completeSweep();
On 2015/06/11 08:49:17, haraken wrote:
> 
> Can we move the completeSweep() into shouldForceMemoryPressureGC()?

It could be done; the alternative is to allow conservative GCs in
scheduleGCIfNeeded() even if sweeping is in progress.

[haraken, 2015-06-11 09:36:24]

On 2015/06/11 08:56:14, sof wrote:
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> Source/platform/heap/ThreadState.cpp:837: completeSweep();
> On 2015/06/11 08:49:17, haraken wrote:
> > 
> > Can we move the completeSweep() into shouldForceMemoryPressureGC()?
> 
> It could be done; the alternative is to allow conservative GCs in
> scheduleGCIfNeeded() even if sweeping is in progress.

I'd prefer calling completeSweep() and then evaluate the threshold (because
there would be no reason to start a new GC while having dead objects that need
to be swept).

[sof, 2015-06-11 09:43:59]

On 2015/06/11 09:36:24, haraken wrote:
> On 2015/06/11 08:56:14, sof wrote:
> >
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> > Source/platform/heap/ThreadState.cpp:837: completeSweep();
> > On 2015/06/11 08:49:17, haraken wrote:
> > > 
> > > Can we move the completeSweep() into shouldForceMemoryPressureGC()?
> > 
> > It could be done; the alternative is to allow conservative GCs in
> > scheduleGCIfNeeded() even if sweeping is in progress.
> 
> I'd prefer calling completeSweep() and then evaluate the threshold (because
> there would be no reason to start a new GC while having dead objects that need
> to be swept).

Yes, but you don't want all v8 major GCs to complete ongoing sweeps. (Or, if you
would like to do that, I'd prefer not going down that unknown road here.)

And if completeSweep() should only be done while under memory pressure, then it
makes to formulate it just like now. Otherwise you end up with a call to a
predicate for the effect, which just looks odd.

[sof, 2015-06-11 09:56:29]

Evaluating performance next (in the background, need to focus elsewhere for the
rest of the day.)

[haraken, 2015-06-11 10:25:41]

On 2015/06/11 09:43:59, sof wrote:
> On 2015/06/11 09:36:24, haraken wrote:
> > On 2015/06/11 08:56:14, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> > > File Source/platform/heap/ThreadState.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1174123002/diff/40001/Source/platform/heap/Th...
> > > Source/platform/heap/ThreadState.cpp:837: completeSweep();
> > > On 2015/06/11 08:49:17, haraken wrote:
> > > > 
> > > > Can we move the completeSweep() into shouldForceMemoryPressureGC()?
> > > 
> > > It could be done; the alternative is to allow conservative GCs in
> > > scheduleGCIfNeeded() even if sweeping is in progress.
> > 
> > I'd prefer calling completeSweep() and then evaluate the threshold (because
> > there would be no reason to start a new GC while having dead objects that
need
> > to be swept).
> 
> Yes, but you don't want all v8 major GCs to complete ongoing sweeps. (Or, if
you
> would like to do that, I'd prefer not going down that unknown road here.)
> 
> And if completeSweep() should only be done while under memory pressure, then
it
> makes to formulate it just like now. Otherwise you end up with a call to a
> predicate for the effect, which just looks odd.

I'm guessing you can do something like:

bool ThreadState::shouldForceMemoryPressureGC() {
  if (current < 300 MB)
    return false;
  completeSweep();
  if (current < 300 MB)
    return false;
  return current > estimated * 3 / 2;
}

?

[haraken, 2015-06-11 10:26:17]

Either way, LGTM.

[sof, 2015-06-11 15:01:56]

The blink_perf results for Oilpan trunk vs this CL,


https://docs.google.com/spreadsheets/d/1uTBkuGVSpvBrveK58D7ZJkXcOiYQrsLCqe504...

shows up no regressions.

dromaeo.domcoremodify is too crashy on ToT to generate results, but for the runs
that did complete, the run/s performance is equal.

I will go ahead and try to land this.

[sof, 2015-06-11 15:02:40]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-11 15:02:59]

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

[commit-bot: I haz the power, 2015-06-11 15:06:24]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196953

[sof, 2015-06-15 14:29:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1191483002/ by sigbjornf@opera.com.

The reason for reverting is: Plausible candidate for regressing v8 GC totals on
v8.top_25_smooth somewhat,

 https://code.google.com/p/chromium/issues/detail?id=499822.