Reland "Proactively purging discardable memory on Desktop"

This is a reland of 3558d80a,
initially reviewed here:
https://chromium-review.googlesource.com/c/chromium/src/+/2321400,
and reverted here:
https://chromium-review.googlesource.com/c/chromium/src/+/2360121

The reverted commit was not responsible for the failure and is safe to
reland, since the build was already passing by the time it was reverted.

(see my comment below for details)

Bug: 1109209
Change-Id: I2df2de93ca97e43044ecf27c40793274c07d9685
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363396Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: Thiabaud Engelbrecht <thiabaud@google.com>
Cr-Commit-Position: refs/heads/master@{#799644}

components/BUILD.gn

@@ -236,6 +236,7 @@ test("components_unittests") {
       "//components/content_settings/browser:unit_tests",
       "//components/contextual_search/core:unit_tests",
       "//components/data_use_measurement/core:unit_tests",
+      "//components/discardable_memory/client:unit_tests",
       "//components/discardable_memory/common:unit_tests",
       "//components/discardable_memory/service:unit_tests",
       "//components/dom_distiller/content/browser:unit_tests",

components/discardable_memory/client/BUILD.gn

@@ -21,3 +21,15 @@ component("client") {
     "//components/discardable_memory/public/mojom",
   ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+
+  sources = [ "client_discardable_shared_memory_manager_unittest.cc" ]
+
+  deps = [
+    ":client",
+    "//base",
+    "//testing/gtest",
+  ]
+}

components/discardable_memory/client/client_discardable_shared_memory_manager.h

@@ -8,9 +8,8 @@
 #include <stddef.h>
 
 #include <memory>
+#include <set>
 
-#include "base/callback_helpers.h"
-#include "base/macros.h"
 #include "base/memory/discardable_memory_allocator.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/unsafe_shared_memory_region.h"
@@ -47,12 +46,16 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
+  // Purge any unlocked memory that was allocated by this manager.
+  void PurgeUnlockedMemory();
+
   // Release memory and associated resources that have been purged.
   void ReleaseFreeMemory() override;
 
-  bool LockSpan(DiscardableSharedMemoryHeap::Span* span);
-  void UnlockSpan(DiscardableSharedMemoryHeap::Span* span);
-  void ReleaseSpan(std::unique_ptr<DiscardableSharedMemoryHeap::Span> span);
+  bool LockSpan(DiscardableSharedMemoryHeap::Span* span)
+      EXCLUSIVE_LOCKS_REQUIRED(GetLock());
+  void UnlockSpan(DiscardableSharedMemoryHeap::Span* span)
+      EXCLUSIVE_LOCKS_REQUIRED(GetLock());
 
   base::trace_event::MemoryAllocatorDump* CreateMemoryAllocatorDump(
       DiscardableSharedMemoryHeap::Span* span,
@@ -70,8 +73,47 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
     bytes_allocated_limit_for_testing_ = limit;
   }
 
+  // We only have protected members for testing, everything else should be
+  // either public or private.
+ protected:
+  explicit ClientDiscardableSharedMemoryManager(
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
+  std::unique_ptr<DiscardableSharedMemoryHeap> heap_ GUARDED_BY(lock_);
+  mutable base::Lock lock_;
+
  private:
-  std::unique_ptr<base::DiscardableSharedMemory>
+  class DiscardableMemoryImpl : public base::DiscardableMemory {
+   public:
+    DiscardableMemoryImpl(
+        ClientDiscardableSharedMemoryManager* manager,
+        std::unique_ptr<DiscardableSharedMemoryHeap::Span> span);
+    ~DiscardableMemoryImpl() override;
+
+    DiscardableMemoryImpl(const DiscardableMemoryImpl&) = delete;
+    DiscardableMemoryImpl& operator=(const DiscardableMemoryImpl&) = delete;
+
+    // Overridden from base::DiscardableMemory:
+    bool Lock() override;
+    void Unlock() override;
+    void* data() const override;
+    void DiscardForTesting() override;
+    base::trace_event::MemoryAllocatorDump* CreateMemoryAllocatorDump(
+        const char* name,
+        base::trace_event::ProcessMemoryDump* pmd) const override;
+
+    // Returns |span_| if unlocked, otherwise nullptr.
+    std::unique_ptr<DiscardableSharedMemoryHeap::Span> Purge()
+        EXCLUSIVE_LOCKS_REQUIRED(manager_->GetLock());
+
+   private:
+    friend class ClientDiscardableSharedMemoryManager;
+    ClientDiscardableSharedMemoryManager* const manager_;
+    std::unique_ptr<DiscardableSharedMemoryHeap::Span> span_;
+    bool is_locked_ GUARDED_BY(manager_->GetLock());
+  };
+
+  // This is only virtual for testing.
+  virtual std::unique_ptr<base::DiscardableSharedMemory>
   AllocateLockedDiscardableSharedMemory(size_t size, int32_t id);
   void AllocateOnIO(size_t size,
                     int32_t id,
@@ -81,18 +123,27 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
                              base::ScopedClosureRunner closure_runner,
                              base::UnsafeSharedMemoryRegion ret_region);
 
-  void DeletedDiscardableSharedMemory(int32_t id);
+  // This is only virtual for testing.
+  virtual void DeletedDiscardableSharedMemory(int32_t id);
   void MemoryUsageChanged(size_t new_bytes_allocated,
                           size_t new_bytes_free) const;
 
+  void ReleaseFreeMemoryImpl();
+  void ReleaseMemory(DiscardableMemoryImpl* memory,
+                     std::unique_ptr<DiscardableSharedMemoryHeap::Span> span)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void ReleaseSpan(std::unique_ptr<DiscardableSharedMemoryHeap::Span> span)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  base::Lock& GetLock() { return lock_; }
+
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
   // TODO(penghuang): Switch to SharedRemote when it starts supporting
   // sync method call.
   std::unique_ptr<mojo::Remote<mojom::DiscardableSharedMemoryManager>>
       manager_mojo_;
 
-  mutable base::Lock lock_;
-  std::unique_ptr<DiscardableSharedMemoryHeap> heap_ GUARDED_BY(lock_);
+  // Holds all locked and unlocked instances which have not yet been purged.
+  std::set<DiscardableMemoryImpl*> allocated_memory_ GUARDED_BY(lock_);
   size_t bytes_allocated_limit_for_testing_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(ClientDiscardableSharedMemoryManager);

components/discardable_memory/client/client_discardable_shared_memory_manager_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/discardable_memory/client/client_discardable_shared_memory_manager.h"
+#include "base/memory/discardable_memory.h"
+#include "base/memory/discardable_shared_memory.h"
+#include "base/process/process_metrics.h"
+#include "base/synchronization/lock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace discardable_memory {
+namespace {
+
+using base::Location;
+using base::OnceClosure;
+using base::TimeDelta;
+
+class TestSingleThreadTaskRunner : public base::SingleThreadTaskRunner {
+  ~TestSingleThreadTaskRunner() override = default;
+  bool PostTask(const Location& from_here, OnceClosure task) { return true; }
+  template <class T>
+  bool DeleteSoon(const Location& from_here, const T* object) {
+    return true;
+  }
+  bool PostDelayedTask(const Location& from_here,
+                       OnceClosure task,
+                       TimeDelta delay) override {
+    return true;
+  }
+  bool PostNonNestableDelayedTask(const Location& from_here,
+                                  OnceClosure task,
+                                  TimeDelta delay) override {
+    return true;
+  }
+  bool RunsTasksInCurrentSequence() const override { return true; }
+};
+
+class TestClientDiscardableSharedMemoryManager
+    : public ClientDiscardableSharedMemoryManager {
+ public:
+  TestClientDiscardableSharedMemoryManager()
+      : ClientDiscardableSharedMemoryManager(
+            base::MakeRefCounted<TestSingleThreadTaskRunner>()) {}
+
+  ~TestClientDiscardableSharedMemoryManager() override = default;
+
+  std::unique_ptr<base::DiscardableSharedMemory>
+  AllocateLockedDiscardableSharedMemory(size_t size, int32_t id) override {
+    auto shared_memory = std::make_unique<base::DiscardableSharedMemory>();
+    shared_memory->CreateAndMap(size);
+    return shared_memory;
+  }
+
+  void DeletedDiscardableSharedMemory(int32_t id) override {}
+
+  size_t GetSize() const {
+    base::AutoLock lock(lock_);
+    return heap_->GetSize();
+  }
+
+  size_t GetSizeOfFreeLists() const {
+    base::AutoLock lock(lock_);
+    return heap_->GetSizeOfFreeLists();
+  }
+};
+
+// This test allocates a single piece of memory, then verifies that calling
+// |PurgeUnlockedMemory| only affects the memory when it is unlocked.
+TEST(ClientDiscardableSharedMemoryManagerTest, Simple) {
+  const size_t page_size = base::GetPageSize();
+  TestClientDiscardableSharedMemoryManager client;
+
+  // Initially, we should have no memory allocated
+  ASSERT_EQ(client.GetBytesAllocated(), 0u);
+  ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
+
+  auto mem = client.AllocateLockedDiscardableMemory(page_size);
+
+  // After allocation, we should have allocated a single piece of memory.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size);
+
+  client.PurgeUnlockedMemory();
+
+  // All our memory is locked, so calling |PurgeUnlockedMemory| should have no
+  // effect.
+  EXPECT_EQ(client.GetBytesAllocated(), base::GetPageSize());
+
+  mem->Unlock();
+
+  // Unlocking has no effect on the amount of memory we have allocated.
+  EXPECT_EQ(client.GetBytesAllocated(), base::GetPageSize());
+
+  client.PurgeUnlockedMemory();
+
+  // Now that |mem| is unlocked, the call to |PurgeUnlockedMemory| will
+  // remove it.
+  EXPECT_EQ(client.GetBytesAllocated(), 0u);
+}
+
+// This test allocates multiple pieces of memory, then unlocks them one by one,
+// verifying that |PurgeUnlockedMemory| only affects the unlocked pieces of
+// memory.
+TEST(ClientDiscardableSharedMemoryManagerTest, MultipleOneByOne) {
+  const size_t page_size = base::GetPageSize();
+  TestClientDiscardableSharedMemoryManager client;
+
+  ASSERT_EQ(client.GetBytesAllocated(), 0u);
+  ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
+
+  auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 2.2);
+  auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 1.1);
+  auto mem3 = client.AllocateLockedDiscardableMemory(page_size * 3.5);
+  auto mem4 = client.AllocateLockedDiscardableMemory(page_size * 0.2);
+
+  EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
+
+  // Does nothing because everything is locked.
+  client.PurgeUnlockedMemory();
+
+  EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
+
+  mem1->Unlock();
+
+  // Does nothing, since we don't have any free memory, just unlocked memory.
+  client.ReleaseFreeMemory();
+
+  EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
+
+  // This gets rid of |mem1| (which is unlocked), but not the rest of the
+  // memory.
+  client.PurgeUnlockedMemory();
+
+  EXPECT_EQ(client.GetBytesAllocated(), 7 * page_size);
+
+  // We do similar checks to above for the rest of the memory.
+  mem2->Unlock();
+
+  client.PurgeUnlockedMemory();
+
+  EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
+
+  mem3->Unlock();
+
+  client.PurgeUnlockedMemory();
+  EXPECT_EQ(client.GetBytesAllocated(), 1 * page_size);
+
+  mem4->Unlock();
+
+  client.PurgeUnlockedMemory();
+  EXPECT_EQ(client.GetBytesAllocated(), 0 * page_size);
+}
+
+// This test allocates multiple pieces of memory, then unlocks them all,
+// verifying that |PurgeUnlockedMemory| only affects the unlocked pieces of
+// memory.
+TEST(ClientDiscardableSharedMemoryManagerTest, MultipleAtOnce) {
+  const size_t page_size = base::GetPageSize();
+  TestClientDiscardableSharedMemoryManager client;
+
+  ASSERT_EQ(client.GetBytesAllocated(), 0u);
+  ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
+
+  auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 2.2);
+  auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 1.1);
+  auto mem3 = client.AllocateLockedDiscardableMemory(page_size * 3.5);
+  auto mem4 = client.AllocateLockedDiscardableMemory(page_size * 0.2);
+
+  EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
+
+  // Does nothing because everything is locked.
+  client.PurgeUnlockedMemory();
+
+  EXPECT_EQ(client.GetBytesAllocated(), 10 * page_size);
+
+  // Unlock all pieces of memory at once.
+  mem1->Unlock();
+  mem2->Unlock();
+  mem3->Unlock();
+  mem4->Unlock();
+
+  client.PurgeUnlockedMemory();
+  EXPECT_EQ(client.GetBytesAllocated(), 0 * page_size);
+}
+
+// Tests that FreeLists are only released once all memory has been released.
+TEST(ClientDiscardableSharedMemoryManagerTest, Release) {
+  const size_t page_size = base::GetPageSize();
+  TestClientDiscardableSharedMemoryManager client;
+
+  ASSERT_EQ(client.GetBytesAllocated(), 0u);
+  ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
+
+  auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 3);
+  auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 2);
+
+  size_t freelist_size = client.GetSizeOfFreeLists();
+  EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
+
+  mem1 = nullptr;
+
+  // Less memory is now allocated, but freelists are grown.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
+
+  client.PurgeUnlockedMemory();
+
+  // Purging doesn't remove any memory since none is unlocked, also doesn't
+  // remove freelists since we still have some.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
+
+  mem2 = nullptr;
+
+  // No memory is allocated, but freelists are grown.
+  EXPECT_EQ(client.GetBytesAllocated(), 0u);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 5);
+
+  client.PurgeUnlockedMemory();
+
+  // Purging now shrinks freelists as well.
+  EXPECT_EQ(client.GetBytesAllocated(), 0u);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), 0u);
+}
+
+// Similar to previous test, but makes sure that freelist still shrinks when
+// last piece of memory was just unlocked instead of released.
+TEST(ClientDiscardableSharedMemoryManagerTest, ReleaseUnlocked) {
+  const size_t page_size = base::GetPageSize();
+  TestClientDiscardableSharedMemoryManager client;
+
+  ASSERT_EQ(client.GetBytesAllocated(), 0u);
+  ASSERT_EQ(client.GetSizeOfFreeLists(), 0u);
+
+  auto mem1 = client.AllocateLockedDiscardableMemory(page_size * 3);
+  auto mem2 = client.AllocateLockedDiscardableMemory(page_size * 2);
+
+  size_t freelist_size = client.GetSizeOfFreeLists();
+  EXPECT_EQ(client.GetBytesAllocated(), 5 * page_size);
+
+  mem1 = nullptr;
+
+  // Less memory is now allocated, but freelists are grown.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
+
+  client.PurgeUnlockedMemory();
+
+  // Purging doesn't remove any memory since none is unlocked, also doesn't
+  // remove freelists since we still have some.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
+
+  mem2->Unlock();
+
+  // No change in memory usage, since memory was only unlocked not released.
+  EXPECT_EQ(client.GetBytesAllocated(), page_size * 2);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), freelist_size + page_size * 3);
+
+  client.PurgeUnlockedMemory();
+
+  // Purging now shrinks freelists as well.
+  EXPECT_EQ(client.GetBytesAllocated(), 0u);
+  EXPECT_EQ(client.GetSizeOfFreeLists(), 0u);
+}
+
+}  // namespace
+}  // namespace discardable_memory