base: Refactor DiscardableMemoryManager for use with different type of allocations.

base/memory/discardable_memory_emulated.cc

 // Copyright 2013 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_emulated.h"
 
+#include "base/bind.h"
 #include "base/lazy_instance.h"
-#include "base/memory/discardable_memory_manager.h"
+#include "base/memory/memory_pressure_listener.h"
+#include "base/message_loop/message_loop.h"
+#include "base/threading/thread_checker.h"
 
 namespace base {
 
 namespace {
 
-base::LazyInstance<internal::DiscardableMemoryManager>::Leaky g_manager =
+class DiscardableMemoryAllocationImpl

[willchan no longer on Chromium, 2014/04/01 01:11:08]

Maybe we should name this DiscardableMemoryHeapAllocation to emphasize what type
of allocation it is? Impl is a bit generic.

[reveman, 2014/04/03 17:02:12]

I added a comment here to emphasize the use of heap memory. I like using the
"Impl" suffix in the anonymous namespace of .cc files. In the case of
discardable_memory_emulated.cc, Type/InterfaceImpl can only be interpreted as
the implementation of Type/Interface for emulated discardable memory. In this
case, it's the implementation of the DiscardableMemoryAllocation type for
emulated discardable memory.

I can change this if you feel strongly about it though.

[willchan no longer on Chromium, 2014/04/15 20:51:31]

I will defer to you here. Please take my comment as purely advisory. I suspect
that it's more obvious to you since you understand the code better that
"emulated" implies emulation using the heap. As an outside reader, it's less
obvious that this is the case, although when I think about it more, it does seem
like the logical conclusion. But I didn't want to have to think about it.

+    : public internal::DiscardableMemoryAllocation {
+ public:
+  explicit DiscardableMemoryAllocationImpl(size_t bytes)
+      : memory_(new uint8[bytes]) {}
+
+  // Overridden from internal::DiscardableMemoryAllocation:
+  virtual bool Lock() OVERRIDE { return true; }
+  virtual void Unlock() OVERRIDE {}
+  virtual void* Memory() OVERRIDE { return memory_.get(); }
+
+ private:
+  scoped_ptr<uint8[]> memory_;
+};
+
+// This is admittedly pretty magical. It's approximately enough memory for four
+// 2560x1600 images.
+const size_t kDefaultDiscardableMemoryLimit = 64 * 1024 * 1024;
+
+// Under moderate memory pressure, we will purge until usage is within this
+// limit.
+const size_t kBytesToKeepUnderModeratePressure =
+    kDefaultDiscardableMemoryLimit / 4;
+
+class DiscardableMemoryManagerImpl
+    : public internal::DiscardableMemoryManager,
+      public internal::DiscardableMemoryAllocation::Factory {
+ public:
+  DiscardableMemoryManagerImpl()
+      : internal::DiscardableMemoryManager(this,

[Philippe, 2014/03/20 18:08:31]

I think this part is slightly hairy :) Correct me if I'm wrong but I believe
that the vptr is not initialized at this point so if
DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call on
|this| (e.g. CreateLockedAllocation()) then the behavior is  undefined. The vptr
gets initialized between the call to the base class' constructor and the
construction of the class' members IIRC.

FWIW I think composition would help here, at least for readability :)

[reveman, 2014/03/20 19:08:22]

Correct.
I think it's OK to rely on the base class not using this in ctor but can change
to a virtual DiscardableMemoryManager::CreateLockedMemory function to make this
more explicit or initialize the factory prior to the manager if you feel
strongly about this.

[Philippe, 2014/03/21 09:13:09]

I think we will have to initialize the factory before the manager if we want to
be consistent across platforms. On Android, the allocator is the factory and
there is no way I can make this class extend both DiscardableMemoryManager and
DiscardableMemoryAllocationFactoryAshmem :)

If you feel strongly about keeping (single) inheritance here vs using
composition you can also make DiscardableMemoryManager take ownership of the
factory. With the current situation (DiscardableMemoryManager taking a raw
pointer to the factory), I have no choice in the Android implementation other
than using composition (which I'm happy with FWIW) :)

[Philippe, 2014/03/21 09:49:19]

I think this should be a reasonable compromise that both you and I could like if
you feel strongly about keeping (single) inheritance:
class DiscardableMemoryAllocationImpl : public DiscardableMemoryAllocation {
 public:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedMemory(
        size_t bytes) OVERRIDE {
      return make_scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(bytes));
    }
  };

  // DiscardableMemoryAllocation:
  // Lock(), Unlock()...
};

class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
 public:
   DiscardableMemoryManagerImpl()
       : DiscardableMemoryManager(
           make_scoped_ptr<DiscardableMemoryAllocation::Factory>(
               new DiscardableMemoryAllocationImpl::Factory()) {
   }
};

I would just find it slightly unfortunate/unusual to have the manager take
ownership of the factory but I can live with it :)

[reveman, 2014/03/21 12:32:16]

Why not?
Sorry, I'm failing to see why. Can you explain?

[reveman, 2014/03/21 12:32:16]

I don't like that either. Awkward for the manager to take ownership of an
instance that implements an interface when it's really just interested in the
interface. Let's not do that.

We can always use a different LazyInstance and instantiate the factory before
the manager impl if we have to. That's much better than passing the ownership
imo.

[Philippe, 2014/03/21 12:40:50]

Because this would be a direct violation of the Liskov substitution principle
just like subclassing the factory here already is. A
DiscardableMemoryManagerImpl is not a Factory (and it's even less an
AshmemFactory). I really think we should avoid such inheritance. I don't see why
you are opposed to composition here? That said having a separate LazyInstance
for the Factory is not ideal but it would already be much better than
subclassing the Factory IMO. What do you think?

[reveman, 2014/03/22 16:49:34]

Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such thing
as "a Factory". There's only the Factory interface that
DiscardableMemoryManagerImpl implements. The use of interfaces throughout the
chromium code is far from rare and this is not doing anything that is not
commonly found in existing chromium code.

To summarize my current thinking; this patch is well aligned with the rest of
the chromium codebase. Passing "this" to the ctor and expecting it not to be
called is not uncommon. Inheriting an interface implementation might be
confusing. Probably better to use composition if DiscardableMemoryManagerImpl
can't implement the interface by itself. Nothing in this patch that prevents
that though.

[Philippe, 2014/03/24 09:35:31]

Just to be clear and so that the information is present here with the full
context, my main concern is not the existence of the Factory interface (although
I would still prefer to replace it with the use of a Callback) but rather the
unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
mentioned before).

Moreover the Google style guide is also pretty clear:
"Do not overuse implementation inheritance. Composition is often more
appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."

I don't know what data you are using to say that this is common in the Chromium
codebase. It might be but I don't think we have ever encouraged/should encourage
such idioms over composition.

If you are concerned about the instance() method in the version of my patch
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
FTR), then we can also use composition with delegate methods, as it should
really be (although it's slightly more verbose). The code below should be safe,
correct and simple IMO.

class DiscardableMemoryManagerEmulated {
 public:
  typedef DiscardableMemoryManager::AllocationId AllocationId;

  DiscardableMemoryManagerEmulated() : manager_(&factory_) {}

  AllocationId Register(size_t size) { return manager_.Register(size); }

  void Unregister(AllocationId id) { manager_.Unregister(id); }  

  void* Lock(AllocationId id, bool* purged) {
    return manager_.Lock(id, purged);
  }

  void Unlock(AllocationId id) { manager_.Unlock(id); }

 private:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
        size_t size) OVERRIDE {
      return scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(size));
    }
  };

  Factory factory_;
  DiscardableMemoryManager manager_;
};

base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
    LAZY_INSTANCE_INITIALIZER;

[willchan no longer on Chromium, 2014/04/01 01:11:08]

I am not fully up to date with Philippe's proposals, but I agree that
implementation inheritance is not desirable. The thing that sticks out to me is
that DiscardableMemoryManagerImpl should not inherit from
DiscardableMemoryManager if DiscardableMemoryManager has a concrete
implementation. Composition is indeed better here IMO.

[reveman, 2014/04/03 17:02:12]

Do we really need to avoid all kinds of concrete class inheritance? Right now
the base class DiscardableMemoryManager, implements the essential parts of the
manager that will be shared across all platforms while this class here extends
it with the memory signal handling we want for emulated discardable memory. I
find this simple and easy to understand and not sure why we'd need to do it
differently unless there's a strict "no concrete class inheritance" policy in
place.

That said, I can change to two different types if that's preferred but we then
need two different names that describe the different roles of these two types
properly. Poorly named classes are worse for readability than some minimal
concrete class inheritance IMO.

[Philippe, 2014/04/14 15:35:47]

I agree, let's move this forward :) I think I have already shared my opinion
here but my favorite alternative FWIW would be the one in my previous comment
which I'm pasting here:

class DiscardableMemoryManagerEmulated {
 public:
  typedef DiscardableMemoryManager::AllocationId AllocationId;

  DiscardableMemoryManagerEmulated() : manager_(&factory_) {}

  AllocationId Register(size_t size) { return manager_.Register(size); }

  void Unregister(AllocationId id) { manager_.Unregister(id); }  

  void* Lock(AllocationId id, bool* purged) {
    return manager_.Lock(id, purged);
  }

  void Unlock(AllocationId id) { manager_.Unlock(id); }

 private:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
        size_t size) OVERRIDE {
      return scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(size));
    }
  };

  Factory factory_;
  DiscardableMemoryManager manager_;
};

base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
    LAZY_INSTANCE_INITIALIZER;

Replacing the factory with a callback would be even better IMO but I would
already be OK with the factory if you still feel strongly about it.

[willchan no longer on Chromium, 2014/04/15 20:51:31]

It's not strictly disallowed, but it's strongly discouraged in the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance ("For
implementation inheritance, because the code implementing a sub-class is spread
between the base and the sub-class, it can be more difficult to understand an
implementation. The sub-class cannot override functions that are not virtual, so
the sub-class cannot change implementation. The base class may also define some
data members, so that specifies physical layout of the base class.") It also
suffers from the Fragile Base Class problem
(http://en.wikipedia.org/wiki/Fragile_base_class).
I am in favor of composition of some form. Philippe's suggestion here seems
reasonable to me. I prefer his solution of defining a private type that
implements the appropriate interface, although sometimes others will choose not
to do this since it does admittedly add some boilerplate. I am more of a
stickler for doing this if the type will be publicly exposed in a header file,
but within a .cc file, I'd be OK avoiding this if that's what you prefer in
order to reduce the need for an extra type. That said, I am more hardline about
not doing implementation inheritance. Composing the DiscardableMemoryManager
instead does feel like the right solution to me.

+                                           kDefaultDiscardableMemoryLimit) {}
+
+  // Overridden from internal::DiscardableMemoryAllocation::Factory:
+  virtual scoped_ptr<internal::DiscardableMemoryAllocation>
+  CreateLockedAllocation(size_t bytes) OVERRIDE {
+    return make_scoped_ptr<internal::DiscardableMemoryAllocation>(
+        new DiscardableMemoryAllocationImpl(bytes));
+  }
+
+  void RegisterMemoryPressureListener() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(base::MessageLoop::current());
+    DCHECK(!memory_pressure_listener_);
+    memory_pressure_listener_.reset(new MemoryPressureListener(base::Bind(
+        &DiscardableMemoryManagerImpl::OnMemoryPressure, Unretained(this))));
+  }
+
+  void UnregisterMemoryPressureListener() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(memory_pressure_listener_);
+    memory_pressure_listener_.reset();
+  }
+
+  // This can be called as a hint that the system is under memory pressure.
+  void OnMemoryPressure(
+      MemoryPressureListener::MemoryPressureLevel pressure_level) {
+    switch (pressure_level) {
+      case MemoryPressureListener::MEMORY_PRESSURE_MODERATE:
+        PurgeUntilUsageIsWithin(kBytesToKeepUnderModeratePressure);
+        return;
+      case MemoryPressureListener::MEMORY_PRESSURE_CRITICAL:
+        PurgeUntilUsageIsWithin(0u);
+        return;
+    }
+
+    NOTREACHED();
+  }
+
+ private:
+  // Allows us to be respond when the system reports that it is under memory
+  // pressure.
+  scoped_ptr<MemoryPressureListener> memory_pressure_listener_;
+
+  base::ThreadChecker thread_checker_;
+};
+base::LazyInstance<DiscardableMemoryManagerImpl>::Leaky g_manager =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
 namespace internal {
 
 DiscardableMemoryEmulated::DiscardableMemoryEmulated(size_t size)
-    : is_locked_(false) {
-  g_manager.Pointer()->Register(this, size);
-}
+    : allocation_id_(g_manager.Pointer()->Register(size)), memory_(NULL) {}
 
 DiscardableMemoryEmulated::~DiscardableMemoryEmulated() {
-  if (is_locked_)
-    Unlock();
-  g_manager.Pointer()->Unregister(this);
+  g_manager.Pointer()->Unregister(allocation_id_);
 }
 
 // static
 void DiscardableMemoryEmulated::RegisterMemoryPressureListeners() {
   g_manager.Pointer()->RegisterMemoryPressureListener();
 }
 
 // static
 void DiscardableMemoryEmulated::UnregisterMemoryPressureListeners() {
   g_manager.Pointer()->UnregisterMemoryPressureListener();
 }
 
 // static
 void DiscardableMemoryEmulated::PurgeForTesting() {
-  g_manager.Pointer()->PurgeAll();
+  g_manager.Pointer()->PurgeUntilUsageIsWithin(0u);
 }
 
 bool DiscardableMemoryEmulated::Initialize() {
   return Lock() == DISCARDABLE_MEMORY_LOCK_STATUS_PURGED;
 }
 
 DiscardableMemoryLockStatus DiscardableMemoryEmulated::Lock() {
-  DCHECK(!is_locked_);
+  DCHECK(!memory_);
 
   bool purged = false;
-  memory_ = g_manager.Pointer()->Acquire(this, &purged);
+  memory_ = g_manager.Pointer()->Lock(allocation_id_, &purged);
   if (!memory_)
     return DISCARDABLE_MEMORY_LOCK_STATUS_FAILED;
 
-  is_locked_ = true;
   return purged ? DISCARDABLE_MEMORY_LOCK_STATUS_PURGED
                 : DISCARDABLE_MEMORY_LOCK_STATUS_SUCCESS;
 }
 
 void DiscardableMemoryEmulated::Unlock() {
-  DCHECK(is_locked_);
-  g_manager.Pointer()->Release(this, memory_.Pass());
-  is_locked_ = false;
+  DCHECK(memory_);
+  g_manager.Pointer()->Unlock(allocation_id_);
+  memory_ = NULL;
 }
 
 void* DiscardableMemoryEmulated::Memory() const {
   DCHECK(memory_);
-  return memory_.get();
+  return memory_;
 }
 
 }  // namespace internal
 }  // namespace base