Reinstate memory pressure Oilpan GC check post V8 major GCs.

Reinstate memory pressure Oilpan GC check post V8 major GCs.

To handle workloads with high allocation rates, where the Oilpan heap
allocations only make up a small fraction compared to what's allocated
by either PartitionAlloc or V8, use V8's major GC notification as
a signal to check for the need to perform an "emergency" memory
pressure conservative GC. Otherwise there might not be enough Oilpan
allocations made to trigger out-of-line heap allocations which trigger
that same check. These GCs and workloads are rare, but reduces the
possibility of running into OOM conditions when Oilpan is faced with
those.

An example where it does show up is Dromaeo's dom-modify.html, which
has a subtest that heavily allocates Text nodes, each holding a longer
string. As each Text node is a small Oilpan heap object, not a lot of heap is
needed for the nodes, but PartitionAlloc's buffer partition size grows much
more sharply and risks signalling OOM. V8's RTS notices the memory pressure
and triggers extra GCs; reuse that signal in Oilpan.

Hence, bring back the check that https://codereview.chromium.org/1190513006/
added for checking this condition.

R=haraken
BUG=474470
Committed: https://crrev.com/431fb544a364ebd80a8db6b03b90cc5a62e4407a
Cr-Commit-Position: refs/heads/master@{#351044}

[sof, 2015-09-25 18:09:28]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-09-25 18:09:28]

please take a look.

[haraken, 2015-09-26 02:04:36]

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

[haraken, 2015-09-26 02:04:37]

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:683: if (gcType ==
V8MajorGC && shouldForceMemoryPressureGC()) {

Is there any reason we want to limit this to V8MajorGC?

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:683: if (gcType ==
V8MajorGC && shouldForceMemoryPressureGC()) {

A better form would be:

if (shouldForceMemoryPressureGC()) {
  completeSweep();
  if (shouldForceMemoryPressureGC()) {
    Heap::collectGarbage(...);
    return;
  }
}

We may want to create a helper method like shouldForceMemoryPressureGCIfNeeded()
that does the work.

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:712: 

It would be better to call shouldForceMemoryPressureGCIfNeeded() here as well. A
follow-up CL is fine.

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:738: if
(shouldForceMemoryPressureGC()) {

I think we should move this up to before the isSweepingInProgress check. Also we
can use shouldForceMemoryPressureGCIfNeeded(). A follow-up CL is fine.

[sof, 2015-09-26 06:17:22]

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:683: if (gcType ==
V8MajorGC && shouldForceMemoryPressureGC()) {
On 2015/09/26 02:04:37, haraken wrote:
> 
> A better form would be:
> 
> if (shouldForceMemoryPressureGC()) {
>   completeSweep();
>   if (shouldForceMemoryPressureGC()) {
>     Heap::collectGarbage(...);
>     return;
>   }
> }
> 
> We may want to create a helper method like
shouldForceMemoryPressureGCIfNeeded()
> that does the work.

We've been over that two-step formulation before in
https://codereview.chromium.org/1174123002/ . Adopted.

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:683: if (gcType ==
V8MajorGC && shouldForceMemoryPressureGC()) {
On 2015/09/26 02:04:37, haraken wrote:
> 
> Is there any reason we want to limit this to V8MajorGC?

v8 will force major GCs on detecting/being signalled memory pressure, so it
doesn't pay to be more vigilant here. That along with being mindful of the v8
perf regressions that were caused last time around when triggering too many
conservative GCs, https://code.google.com/p/chromium/issues/detail?id=499822

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:683: if (gcType ==
V8MajorGC && shouldForceMemoryPressureGC()) {
On 2015/09/26 02:04:37, haraken wrote:
> 
> A better form would be:
> 
> if (shouldForceMemoryPressureGC()) {
>   completeSweep();
>   if (shouldForceMemoryPressureGC()) {
>     Heap::collectGarbage(...);
>     return;
>   }
> }
> 
> We may want to create a helper method like
shouldForceMemoryPressureGCIfNeeded()
> that does the work.

Done.

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:712: 
On 2015/09/26 02:04:37, haraken wrote:
> 
> It would be better to call shouldForceMemoryPressureGCIfNeeded() here as well.
A
> follow-up CL is fine.

Could you explain your underlying reasoning?

(We perform completeSweep() just above + it is likely that there will be an
out-of-line allocation to follow soon hereafter.)

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:738: if
(shouldForceMemoryPressureGC()) {
On 2015/09/26 02:04:37, haraken wrote:
> 
> I think we should move this up to before the isSweepingInProgress check. Also
we
> can use shouldForceMemoryPressureGCIfNeeded(). A follow-up CL is fine.

(ftr, such a move on its own is not sufficient to handle the OOM on
dom-modify.html )

[haraken, 2015-09-26 12:38:20]

LGTM

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:712: 
On 2015/09/26 06:17:22, sof wrote:
> On 2015/09/26 02:04:37, haraken wrote:
> > 
> > It would be better to call shouldForceMemoryPressureGCIfNeeded() here as
well.
> A
> > follow-up CL is fine.
> 
> Could you explain your underlying reasoning?
> 
> (We perform completeSweep() just above + it is likely that there will be an
> out-of-line allocation to follow soon hereafter.)

My reasoning is just that it would be better to always check a possibility of a
memory pressure GC.

https://codereview.chromium.org/1369783002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:738: if
(shouldForceMemoryPressureGC()) {
On 2015/09/26 06:17:22, sof wrote:
> On 2015/09/26 02:04:37, haraken wrote:
> > 
> > I think we should move this up to before the isSweepingInProgress check.
Also
> we
> > can use shouldForceMemoryPressureGCIfNeeded(). A follow-up CL is fine.
> 
> (ftr, such a move on its own is not sufficient to handle the OOM on
> dom-modify.html )

Yes, my point is just that it will be better in theory :)

[sof, 2015-09-28 06:04:25]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-09-28 06:04:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369783002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369783002/20001

[commit-bot: I haz the power, 2015-09-28 09:29:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-28 09:30:29]

Patchset 2 (id:??) landed as
https://crrev.com/431fb544a364ebd80a8db6b03b90cc5a62e4407a
Cr-Commit-Position: refs/heads/master@{#351044}