Android: Free GLHelper context ashmem when it makes sense

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index b90a8cf51421d8b01c9d85ce23ad6f8fff351b20..4077e611da76ac06bffe45c3d7e9492cfeb35eb7 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -8,6 +8,7 @@
 
 #include <utility>
 
+#include "base/android/application_status_listener.h"
 #include "base/android/build_info.h"
 #include "base/android/context_utils.h"
 #include "base/bind.h"
@@ -16,6 +17,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/memory_pressure_listener.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
@@ -99,6 +101,10 @@ const int kUndefinedOutputSurfaceId = -1;
 
 static const char kAsyncReadBackString[] = "Compositing.CopyFromSurfaceTime";
 
+using base::android::ApplicationState;
+using base::android::ApplicationStatusListener;
+using base::MemoryPressureListener;
+
 class GLHelperHolder {
  public:
   static GLHelperHolder* Create();
@@ -110,17 +116,33 @@ class GLHelperHolder {
     return provider_->ContextGL()->GetGraphicsResetStatusKHR() != GL_NO_ERROR;
   }
 
+  void FreeUnusedSharedMemoryIfNeeded();
+
  private:
-  GLHelperHolder() = default;
+  GLHelperHolder();
   void Initialize();
   void OnContextLost();
+  void OnApplicationStatusChanged(ApplicationState new_state);
+  void OnMemoryPressure(MemoryPressureListener::MemoryPressureLevel level);
 
   scoped_refptr<ContextProviderCommandBuffer> provider_;
   std::unique_ptr<display_compositor::GLHelper> gl_helper_;
 
+  bool has_running_activities_;
+  bool should_aggressively_free_memory_;
+
+  std::unique_ptr<ApplicationStatusListener> app_status_listener_;
+  std::unique_ptr<MemoryPressureListener> mem_pressure_listener_;
+
   DISALLOW_COPY_AND_ASSIGN(GLHelperHolder);
 };
 
+GLHelperHolder::GLHelperHolder()
+    : has_running_activities_(
+          ApplicationStatusListener::GetState() ==
+          base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES),
+      should_aggressively_free_memory_(!has_running_activities_) {}
+
 GLHelperHolder* GLHelperHolder::Create() {
   GLHelperHolder* holder = new GLHelperHolder;
   holder->Initialize();
@@ -183,20 +205,59 @@ void GLHelperHolder::Initialize() {
       base::Bind(&GLHelperHolder::OnContextLost, base::Unretained(this)));
   gl_helper_.reset(new display_compositor::GLHelper(
       provider_->ContextGL(), provider_->ContextSupport()));
+
+  app_status_listener_.reset(new ApplicationStatusListener(base::Bind(
+      &GLHelperHolder::OnApplicationStatusChanged, base::Unretained(this))));

[danakj, 2016/09/13 22:44:58]

Please leave a comment explaining why unretained this is safe

[no sievers, 2016/09/26 22:19:28]

Done.

+  mem_pressure_listener_.reset(new MemoryPressureListener(base::Bind(
+      &GLHelperHolder::OnMemoryPressure, base::Unretained(this))));

[danakj, 2016/09/13 22:44:58]

ditto

[no sievers, 2016/09/26 22:19:28]

Done.

+}
+
+void GLHelperHolder::FreeUnusedSharedMemoryIfNeeded() {
+  if (should_aggressively_free_memory_)
+    provider_->FreeUnusedSharedMemory();
 }
 
 void GLHelperHolder::OnContextLost() {
+  app_status_listener_.reset();
+  mem_pressure_listener_.reset();
+  provider_->FreeUnusedSharedMemory();

[danakj, 2016/09/13 22:44:58]

What if we just delete the helper instead?

[no sievers, 2016/09/26 22:19:28]

Yes, good idea and it works because IsLost() above will handle |gl_helper_ ==
null| and we'd recreate it lazily from the getter below.

   // Need to post a task because the command buffer client cannot be deleted
   // from within this callback.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&RenderWidgetHostViewAndroid::OnContextLost));
 }
 
-// This can only be used for readback postprocessing. It may return null if the
-// channel was lost and not reestablished yet.
-display_compositor::GLHelper* GetPostReadbackGLHelper() {
+void GLHelperHolder::OnApplicationStatusChanged(ApplicationState new_state) {
+  bool new_has_running_activities =
+      new_state == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES;
+
+  if (!has_running_activities_ && new_has_running_activities) {
+    // Intentionally allow reset at this time even if we previously had a
+    // low memory signal.
+    should_aggressively_free_memory_ = false;
+  } else if (!new_has_running_activities) {
+    should_aggressively_free_memory_ = true;
+    provider_->FreeUnusedSharedMemory();
+  }
+
+  has_running_activities_ = new_has_running_activities;
+}
+
+void GLHelperHolder::OnMemoryPressure(
+    MemoryPressureListener::MemoryPressureLevel level) {
+  if (level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE ||
+      level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) {
+    should_aggressively_free_memory_ = true;

[danakj, 2016/09/13 22:44:58]

I feel weird about 2 functions setting this independently, will this lead to
some weird behaviour where one sets it and the other changes it and we get not
what we hoped for?

[no sievers, 2016/09/26 22:19:28]

Done.

+    provider_->FreeUnusedSharedMemory();
+  }
+}
+
+GLHelperHolder* GetPostReadbackGLHelperHolder(bool create_if_necessary) {
   static GLHelperHolder* g_readback_helper_holder = nullptr;
 
+  if (!create_if_necessary && !g_readback_helper_holder)
+    return nullptr;
+
   if (g_readback_helper_holder && g_readback_helper_holder->IsLost()) {
     delete g_readback_helper_holder;
     g_readback_helper_holder = nullptr;
@@ -205,7 +266,19 @@ display_compositor::GLHelper* GetPostReadbackGLHelper() {
   if (!g_readback_helper_holder)
     g_readback_helper_holder = GLHelperHolder::Create();
 
-  return g_readback_helper_holder->gl_helper();
+  return g_readback_helper_holder;
+}
+
+display_compositor::GLHelper* GetPostReadbackGLHelper() {
+  return GetPostReadbackGLHelperHolder(true)->gl_helper();

[danakj, 2016/09/13 22:44:58]

can you put the true into a temp var to give it a name

[no sievers, 2016/09/26 22:19:29]

Done.

+}
+
+void FreeUnusedGLHelperMemory() {
+  GLHelperHolder* holder = GetPostReadbackGLHelperHolder(false);

[danakj, 2016/09/13 22:44:58]

ditto for false

[no sievers, 2016/09/26 22:19:28]

Done.

+
+  if (holder) {
+    holder->FreeUnusedSharedMemoryIfNeeded();
+  }
 }
 
 void CopyFromCompositingSurfaceFinished(
@@ -224,6 +297,9 @@ void CopyFromCompositingSurfaceFinished(
     if (gl_helper)
       gl_helper->GenerateSyncToken(&sync_token);
   }
+
+  FreeUnusedGLHelperMemory();

[danakj, 2016/09/13 22:44:58]

What if there was more than one copy going?

[no sievers, 2016/09/26 22:19:28]

I think that's fine because it would only free *unused* memory until the last
readback completes and we'd free the remainder.

+
   const bool lost_resource = !sync_token.HasData();
   release_callback->Run(sync_token, lost_resource);
   UMA_HISTOGRAM_TIMES(kAsyncReadBackString,