[Not for review] Discardable memory emulation

base/memory/discardable_memory_emulated.cc

Index: base/memory/discardable_memory_emulated.cc
diff --git a/base/memory/discardable_memory_emulated.cc b/base/memory/discardable_memory_emulated.cc
new file mode 100644
index 0000000000000000000000000000000000000000..17f56360e20f4dd92ec10417582f0b841642e3d4
--- /dev/null
+++ b/base/memory/discardable_memory_emulated.cc
@@ -0,0 +1,213 @@
+// Copyright (c) 2012 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 "base/memory/discardable_memory.h"
+
+#include "base/containers/hash_tables.h"
+#include "base/containers/mru_cache.h"
+#include "base/memory/singleton.h"
+#include "base/synchronization/lock.h"
+#include "base/sys_info.h"
+
+#if defined(COMPILER_GCC)
+namespace BASE_HASH_NAMESPACE {
+template <>
+struct hash<base::DiscardableMemory*> {
+  size_t operator()(base::DiscardableMemory* ptr) const {
+    return hash<size_t>()(reinterpret_cast<size_t>(ptr));
+  }
+};
+}  // namespace BASE_HASH_NAMESPACE
+#endif  // COMPILER
+
+namespace base {
+
+namespace {
+
+// These are definitely magical. Is there a nice way to set them?
+static size_t kCacheLimitUpperBound = 32 * 1024 * 1024;
+static size_t kCacheLimitPurgeBound = 4 * 1024 * 1024;
+static int64 kSystemMemoryLowerBound = 256 * 1024 * 1024;
+
+static bool SystemIsUnderMemoryPressure() {
+  return SysInfo::AmountOfAvailablePhysicalMemory() <
+      kSystemMemoryLowerBound;

[Avi (use Gerrit), 2013/06/12 14:56:49]

Way too magical. Does this function belong in SysInfo? I'm borderline here, but
would be OK if you pushed hard.

+}
+
+typedef HashingMRUCache<DiscardableMemory*, bool> AllocationMap;

[Avi (use Gerrit), 2013/06/12 14:56:49]

bool? If that's just a placeholder, say so.

+
+}  // namespace
+
+// This provider is accessed from multiple threads, so some care needs to be
+// taken to ensure thread safety. In general, the policy has been that
+// allocations are initiated by client code (via Lock or InitializeAndLock),
+// and deallocations are initiated by the provider. Given this policy, the
+// provider assumes that the lock has already been aquired by the provider
+// itself in the Purge/Policy/DidDeallocate methods, but does not make this
+// assumption in the rest of the interface and aquires the lock as appropriate.
+class DiscardableMemoryProvider {

[Avi (use Gerrit), 2013/06/12 14:56:49]

Give the member functions explanatory comments. Member functions love
explanatory comments.

[Avi (use Gerrit), 2013/06/12 14:56:49]

DiscardableMemoryProvider desperately needs a unit test. The current unittest
file tests basic functionality of a system out of our control, but if we control
DiscardableMemoryProvider, we should be able to put it through a much more
thorough evaluation.

+ public:
+  DiscardableMemoryProvider()
+      : allocations_(AllocationMap::NO_AUTO_EVICT),
+        bytes_allocated_(0) {
+  }
+
+  virtual ~DiscardableMemoryProvider() {
+    AutoLock lock(lock_);
+    AllocationMap::iterator it = allocations_.begin();
+    for (; it != allocations_.end(); ++it)
+      if (it->first->Memory())
+        it->first->Deallocate();
+  }
+
+  static DiscardableMemoryProvider* GetInstance() {
+    return Singleton<DiscardableMemoryProvider>::get();
+  }
+
+  void Register(DiscardableMemory* discardable) {
+    Unregister(discardable);

[Avi (use Gerrit), 2013/06/12 14:56:49]

Why the Unregister? Register is only called from
DiscardableMemory::InitializeAndLock below, which is an error to call more than
once, so it shouldn't ever be in the allocation cache. Definitely DCHECK if you
want, but Unregistering seems useless.

+    {
+      AutoLock lock(lock_);
+      allocations_.Put(discardable, true);
+      EnforcePolicy();
+    }
+  }
+
+  void Unregister(DiscardableMemory* discardable) {
+    if (discardable->is_locked())
+      discardable->Unlock();
+    {
+      AutoLock lock(lock_);
+      if (discardable->memory_)
+        discardable->Deallocate();

[Avi (use Gerrit), 2013/06/12 14:56:49]

Eh...

Unregister is only called by ~DiscardableMemory, and it's weird that the
destructor of the class calls into a different class, which then calls back into
the class to dealloc. Can we move this call to DiscardableMemory::Deallocate()
into ~DiscardableMemory?

+      AllocationMap::iterator it = allocations_.Peek(discardable);
+      if (it != allocations_.end()) {
+        allocations_.Erase(it);
+        EnforcePolicy();
+      }
+    }
+  }
+
+  void DidAllocate(size_t bytes) {
+    AutoLock lock(lock_);
+    bytes_allocated_ += bytes;
+    EnforcePolicy();
+  }
+
+  void DidDeallocate(size_t bytes) {
+    lock_.AssertAcquired();
+    DCHECK(bytes <= bytes_allocated_);
+    bytes_allocated_ -= bytes;
+    EnforcePolicy();
+  }
+
+  void DidAccess(DiscardableMemory* discardable) {
+    AutoLock lock(lock_);
+    allocations_.Get(discardable);
+  }
+
+  void ForcePurge() {
+    AutoLock lock(lock_);
+    PurgeAll();
+  }
+
+  void PurgeAll() {
+    AllocationMap::iterator it = allocations_.begin();
+    for (; it != allocations_.end(); ++it) {
+      if (it->first->memory_ && !it->first->is_locked()) {
+        it->first->Deallocate();
+        DCHECK(!it->first->memory_);
+      }
+    }
+  }
+
+  void PurgeLRU() {
+    AllocationMap::reverse_iterator it = allocations_.rbegin();
+    for(; it != allocations_.rend(); ++it) {
+      if (!it->first->memory_ || !it->first->is_locked())
+        continue;
+      it->first->Deallocate();
+      DCHECK(!it->first->memory_);
+      if (bytes_allocated_ < kCacheLimitPurgeBound)

[cpu_(ooo_6.6-7.5), 2013/06/13 20:16:06]

I was expecting freeing N megs, not freeing until N megs total

+        break;
+    }
+  }
+
+  void EnforcePolicy() {
+    if (SystemIsUnderMemoryPressure())
+      PurgeAll();

[cpu_(ooo_6.6-7.5), 2013/06/13 20:16:06]

again, surprised on the full purge.

+    else if (bytes_allocated_ > kCacheLimitUpperBound)
+      PurgeLRU();
+  }
+
+ private:
+  AllocationMap allocations_;
+  bool visible_;
+  size_t bytes_allocated_;
+  Lock lock_;

[Avi (use Gerrit), 2013/06/12 14:56:49]

Member variables need comment love too. In particular, what is lock_ locking?

+
+  DISALLOW_COPY_AND_ASSIGN(DiscardableMemoryProvider);
+};
+
+DiscardableMemory::~DiscardableMemory() {
+  DiscardableMemoryProvider::GetInstance()->Unregister(this);
+}
+
+bool DiscardableMemory::InitializeAndLock(size_t size) {
+  DiscardableMemoryProvider::GetInstance()->Register(this);
+  size_ = size;
+  return Lock() != DISCARDABLE_MEMORY_FAILED;

[Avi (use Gerrit), 2013/06/12 14:56:49]

LockDiscardableMemoryStatus status = Lock();
DCHECK_NE(DISCARDABLE_MEMORY_SUCCESS, status);  // InitializeAndLock should only
be called once; why is there already memory?
return status == DISCARDABLE_MEMORY_PURGED;

+}
+
+LockDiscardableMemoryStatus DiscardableMemory::Lock() {
+  DCHECK(!is_locked_);
+  DiscardableMemoryProvider::GetInstance()->DidAccess(this);
+  is_locked_ = true;
+
+  if (memory_)
+    return DISCARDABLE_MEMORY_SUCCESS;
+
+  if (Allocate())
+    return DISCARDABLE_MEMORY_PURGED;
+
+  return DISCARDABLE_MEMORY_FAILED;
+}
+
+void DiscardableMemory::Unlock() {
+  DCHECK(is_locked_);
+  DiscardableMemoryProvider::GetInstance()->DidAccess(this);
+  is_locked_ = false;
+}
+
+bool DiscardableMemory::Allocate() {
+  DCHECK(!memory_);
+  memory_ = malloc(size_ * sizeof(char));

[Avi (use Gerrit), 2013/06/12 14:56:49]

sizeof(char) is defined by the standard to be 1 (C++98 §5.3.3¶1, expr.sizeof),
so just do |malloc(size_)|.

+  if (memory_)
+    DiscardableMemoryProvider::GetInstance()->DidAllocate(size_);
+  return memory_;
+}
+
+void DiscardableMemory::Deallocate() {
+  DCHECK(memory_);
+  free(memory_);
+  memory_ = NULL;
+  DiscardableMemoryProvider::GetInstance()->DidDeallocate(size_);
+}
+
+
+// static
+bool DiscardableMemory::PurgeForTestingSupported() {
+  return true;
+}
+
+// static
+void DiscardableMemory::PurgeForTesting() {
+  Purge();
+}
+
+void DiscardableMemory::Purge() {
+  DiscardableMemoryProvider::GetInstance()->ForcePurge();
+}
+
+}  // namespace base