Reinstate memory pressure Oilpan GC check post V8 major GCs.

third_party/WebKit/Source/platform/heap/ThreadState.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 660 matching lines @@
 }
 
 void ThreadState::scheduleV8FollowupGCIfNeeded(V8GCType gcType)
 {
     ASSERT(checkThread());
     Heap::reportMemoryUsageForTracing();
 
     if (isGCForbidden())
         return;
 
+    // If V8 has acted on a memory pressure signal and performed a major GC,
+    // follow up, if needed.
+    if (gcType == V8MajorGC && shouldForceMemoryPressureGC()) {

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

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.

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

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

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

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

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

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

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

Done.

+        Heap::collectGarbage(HeapPointersOnStack, GCWithoutSweep, Heap::ConservativeGC);
+        return;
+    }
+
     if (isSweepingInProgress())
         return;
     ASSERT(!sweepForbidden());
 
     if (shouldScheduleV8FollowupGC())
         schedulePreciseGC();
     else if (gcType == V8MinorGC)
         scheduleIdleGC();
 }
 
 void ThreadState::schedulePageNavigationGCIfNeeded(float estimatedRemovalRatio)
 {
     ASSERT(checkThread());
     Heap::reportMemoryUsageForTracing();
 
     if (isGCForbidden())
         return;
 
     // Finish on-going lazy sweeping.
     // TODO(haraken): It might not make sense to force completeSweep() for all
     // page navigations.
     completeSweep();
     ASSERT(!isSweepingInProgress());
     ASSERT(!sweepForbidden());
 

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

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

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

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.)

[haraken, 2015/09/26 12:38:19]

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

     if (shouldSchedulePageNavigationGC(estimatedRemovalRatio))
         schedulePageNavigationGC();
 }
 
 void ThreadState::schedulePageNavigationGC()
 {
     ASSERT(checkThread());
     ASSERT(!isSweepingInProgress());
     setGCState(PageNavigationGCScheduled);
 }
 
 void ThreadState::scheduleGCIfNeeded()
 {
     ASSERT(checkThread());
     Heap::reportMemoryUsageForTracing();
 
     if (isGCForbidden())
         return;
 
     // Allocation is allowed during sweeping, but those allocations should not
     // trigger nested GCs.
     if (isSweepingInProgress())
         return;
     ASSERT(!sweepForbidden());
 
     if (shouldForceMemoryPressureGC()) {

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

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]

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

[haraken, 2015/09/26 12:38:19]

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

         Heap::collectGarbage(HeapPointersOnStack, GCWithoutSweep, Heap::ConservativeGC);
         return;
     }
     if (shouldForceConservativeGC()) {
         Heap::collectGarbage(HeapPointersOnStack, GCWithoutSweep, Heap::ConservativeGC);
         return;
     }
     if (shouldSchedulePreciseGC())
         schedulePreciseGC();
     else if (shouldScheduleIdleGC())
@@ skipping 863 matching lines @@
         json->beginArray(it->key.ascii().data());
         for (size_t age = 0; age <= maxHeapObjectAge; ++age)
             json->pushInteger(it->value.ages[age]);
         json->endArray();
     }
     TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("blink_gc"), statsName, this, json.release());
 }
 #endif
 
 } // namespace blink