Do V8 GC ASAP if system memory is pressured

content/renderer/render_thread_impl.cc

Index: content/renderer/render_thread_impl.cc
diff --git a/content/renderer/render_thread_impl.cc b/content/renderer/render_thread_impl.cc
index a1fbd8716628ab7dd0ba79704a3876cb6c9f3019..d978783f430844c81346935d150d3065f42f2991 100644
--- a/content/renderer/render_thread_impl.cc
+++ b/content/renderer/render_thread_impl.cc
@@ -163,7 +163,6 @@
 #include "third_party/skia/include/core/SkGraphics.h"
 #include "ui/base/layout.h"
 #include "ui/base/ui_base_switches.h"
-#include "v8/include/v8.h"
 
 #if defined(OS_ANDROID)
 #include <cpu-features.h>
@@ -814,7 +813,9 @@ void RenderThreadImpl::Init() {
 #endif
 
   memory_pressure_listener_.reset(new base::MemoryPressureListener(
-      base::Bind(&RenderThreadImpl::OnMemoryPressure, base::Unretained(this))));
+      base::Bind(&RenderThreadImpl::OnMemoryPressure, base::Unretained(this)),
+      base::Bind(&RenderThreadImpl::OnSyncMemoryPressure,
+                 base::Unretained(this))));
 
   int num_raster_threads = 0;
   std::string string_value =
@@ -2005,6 +2006,7 @@ void RenderThreadImpl::OnCreateNewSharedWorker(
 
 void RenderThreadImpl::OnMemoryPressure(
     base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {
+  TRACE_EVENT0("memory","RenderThreadImpl::OnMemoryPressure");
   ReleaseFreeMemory();
 
   // Do not call into blink if it is not initialized.
@@ -2013,11 +2015,6 @@ void RenderThreadImpl::OnMemoryPressure(
         static_cast<blink::WebMemoryPressureLevel>(memory_pressure_level));
 
     if (blink::mainThreadIsolate()) {
-      // Trigger full v8 garbage collection on memory pressure notifications.
-      // This will potentially hang the renderer for a long time, however, when
-      // we receive a memory pressure notification, we might be about to be
-      // killed.
-      blink::mainThreadIsolate()->LowMemoryNotification();
       RenderThread::Get()->PostTaskToAllWebWorkers(
           base::Bind(&LowMemoryNotificationOnThisThread));
     }
@@ -2176,4 +2173,24 @@ void RenderThreadImpl::PendingRenderFrameConnect::OnConnectionError() {
   DCHECK_EQ(1u, erased);
 }
 
+void RenderThreadImpl::OnSyncMemoryPressure(
+    base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {
+  v8::MemoryPressureLevel memoryPressureLevel =

[esprehn, 2016/03/25 09:20:35]

v8_memory_pressure_level or something of that nature, content uses hacker_case
variables.

[hong.zheng, 2016/04/01 09:22:34]

Done.

+      v8::MemoryPressureLevel::kNone;
+  switch (memory_pressure_level) {

[jochen (gone - plz use gerrit), 2016/03/30 16:23:01]

this should probably also take into account whether the renderer is in the
foreground or not?

[hong.zheng, 2016/04/01 09:22:34]

The CL can deliver memory pressure notification to V8 directly and V8 only does
immediate GC for critical notification. Considering that critical notification
means background process will be killed and foreground process also has OOM
risk, such as BUG=590975. So maybe we need deliver memory pressure notification
to V8 in foreground.

[jochen (gone - plz use gerrit), 2016/04/03 11:21:35]

V8 doesn't look at whether the renderer is in the foreground or not. We should
translate a android memory pressure event + (foreground state) to the
appropriate v8 memory pressure event here.

[hong.zheng, 2016/04/05 09:09:17]

It is a problem about how to balance OOM risk and performance. There are two
solutions:
1. use the same memory pressure level as chromium in V8 for foreground renderer
and translate chromium critical and moderate level to V8 critical level for
background renderer. 
2. translate chromium critical and moderate level to V8 moderate level for
foreground renderer and use the same memory pressure level as chromium in V8 for
background renderer
which one do you think more make sense?

[hong.zheng, 2016/04/07 10:55:49]

Done.

+    case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
+      memoryPressureLevel = v8::MemoryPressureLevel::kNone;

[esprehn, 2016/03/25 09:20:35]

Sigh, not having base in v8 makes for really silly code. Why does base/ use -1,
0, 2 instead of regular enum indexing for this? If the enums were parallel we
could just static_assert and cast between them.

[hong.zheng, 2016/04/01 09:22:34]

Done.

+      break;
+    case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:
+      memoryPressureLevel = v8::MemoryPressureLevel::kModerate;
+      break;
+    case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
+      memoryPressureLevel = v8::MemoryPressureLevel::kCritical;
+      break;
+    default:
+      NOTREACHED();
+  }
+  blink::mainThreadIsolate()->MemoryPressureNotification(memoryPressureLevel);

[jochen (gone - plz use gerrit), 2016/03/30 16:22:23]

since this API is threadsafe, why not notify the workers as well?

[hong.zheng, 2016/04/01 09:22:34]

Done.

+}
+
 }  // namespace content