base: Discardable memory types.

base/memory/discardable_memory_android.cc

Index: base/memory/discardable_memory_android.cc
diff --git a/base/memory/discardable_memory_android.cc b/base/memory/discardable_memory_android.cc
index 7e84967055f1ba9d35f8c6efaeb84b12833586ad..e127047685daf79f8ac9d5fd337e0a0f538fa7f1 100644
--- a/base/memory/discardable_memory_android.cc
+++ b/base/memory/discardable_memory_android.cc
@@ -18,6 +18,7 @@
 #include "base/logging.h"
 #include "base/memory/discardable_memory.h"
 #include "base/memory/discardable_memory_allocator_android.h"
+#include "base/memory/discardable_memory_emulated.h"
 #include "base/synchronization/lock.h"
 #include "third_party/ashmem/ashmem.h"
 
@@ -118,6 +119,16 @@ bool CheckSizeCanBeAlignedToNextPage(size_t size) {
   return size <= std::numeric_limits<size_t>::max() - kPageSize + 1;
 }
 
+struct SupportedTypeVector {

[Philippe, 2013/12/17 14:28:21]

This is just a suggestion but I see that we have now this struct duplicated in
many files with the LazyInstance and GetSupportedTypes() too and I see at the
same time that you now have discardable_memory.cc which looks like a good place
to me to have those details.

So I was thinking that maybe you could declare somewhere (e.g. at the bottom of
discardable_memory.h or maybe in discardable_memory_internal.h):
namespace internal {

void
InitializeSupportedDiscardableMemoryTypes(std::vector<DiscardableMemoryType>*
types);

const std::vector<DiscardableMemoryType>& GetSupportedDiscardableMemoryTypes();

}

DiscardableMemory::GetType() and DiscardableMemory::SetType() could possibly be
moved to that internal namespace too since I don't expect most of the clients to
mess up with that.

And then you would have the DiscardableMemory implementations define
InitializeSupportedTypes() like this for instance:
void InitializeSupportedTypes(std::vector<DiscardableMemoryType>* types) {
  types->push_back(DISCARDABLE_MEMORY_ANDROID);
  types->push_back(DISCARDABLE_MEMORY_EMULATED);
}

Then in discardable_memory.cc you would have:
namespace {

struct GlobalContext {
  GlobalContext() {
    internal::InitializeSupportedTypes(&supported_types);
    DCHECK(!supported_types.empty());
    current_type = supported_types.front();
  }

  std::vector<DiscardableMemoryType> supported_types;
  DiscardableMemoryType current_type;
};

base::LazyInstance<GlobalContext> g_context...

const std::vector<DiscardableMemoryType>& GetSupportedDiscardableMemoryTypes() {
  return g_context.Get().supported_types;
}

} 

What do you think?

[reveman, 2013/12/18 08:12:38]

Please have a look at the latest code. I simply made GetSupportedTypes() take a
pointer to a vector instead. The only code that prefer something else is the
unit test code and small utility function solved that.

[Philippe, 2013/12/18 09:07:47]

Yeah, even better!

+  SupportedTypeVector() {
+    v.push_back(DISCARDABLE_MEMORY_ANDROID);
+    v.push_back(DISCARDABLE_MEMORY_EMULATED);
+  }
+  std::vector<DiscardableMemoryType> v;
+};
+LazyInstance<SupportedTypeVector>::Leaky g_supported_types =
+    LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 namespace internal {
@@ -199,8 +210,9 @@ bool UnlockAshmemRegion(int fd, size_t off, size_t size, const void* address) {
 }  // namespace internal
 
 // static
-bool DiscardableMemory::SupportedNatively() {
-  return true;
+const std::vector<DiscardableMemoryType>&
+    DiscardableMemory::GetSupportedTypes() {
+  return g_supported_types.Get().v;
 }
 
 // Allocation can happen in two ways:
@@ -221,29 +233,48 @@ bool DiscardableMemory::SupportedNatively() {
 // static
 scoped_ptr<DiscardableMemory> DiscardableMemory::CreateLockedMemory(
     size_t size) {
-  if (!CheckSizeCanBeAlignedToNextPage(size))
-    return scoped_ptr<DiscardableMemory>();
-  // Pinning & unpinning works with page granularity therefore align the size
-  // upfront.
-  const size_t aligned_size = internal::AlignToNextPage(size);
-  // Note that the following code is slightly racy. The worst that can happen in
-  // practice though is taking the wrong decision (e.g. using the allocator
-  // rather than DiscardableMemoryAndroidSimple). Moreover keeping the lock
-  // acquired for the whole allocation would cause a deadlock when the allocator
-  // tries to create an ashmem region.
-  const size_t kAllocatorRegionSize =
-      internal::DiscardableMemoryAllocator::kMinAshmemRegionSize;
-  GlobalContext* const global_context = g_context.Pointer();
-  if (aligned_size >= kAllocatorRegionSize ||
-      GetCurrentNumberOfAshmemFDs() < 0.9 * global_context->ashmem_fd_limit) {
-    int fd;
-    void* address;
-    if (internal::CreateAshmemRegion("", aligned_size, &fd, &address)) {
-      return scoped_ptr<DiscardableMemory>(
-          new DiscardableMemoryAndroidSimple(fd, address, aligned_size));
+  switch (GetType()) {
+    case DISCARDABLE_MEMORY_NONE:
+    case DISCARDABLE_MEMORY_MAC:
+      return scoped_ptr<DiscardableMemory>();
+    case DISCARDABLE_MEMORY_ANDROID: {
+      if (!CheckSizeCanBeAlignedToNextPage(size))
+        return scoped_ptr<DiscardableMemory>();
+      // Pinning & unpinning works with page granularity therefore align the
+      // size upfront.
+      const size_t aligned_size = internal::AlignToNextPage(size);
+      // Note that the following code is slightly racy. The worst that can
+      // happen in practice though is taking the wrong decision (e.g. using
+      // the allocator rather than DiscardableMemoryAndroidSimple). Moreover
+      // keeping the lock acquired for the whole allocation would cause a
+      // deadlock when the allocator tries to create an ashmem region.
+      const size_t kAllocatorRegionSize =
+          internal::DiscardableMemoryAllocator::kMinAshmemRegionSize;
+      GlobalContext* const global_context = g_context.Pointer();
+      if (aligned_size >= kAllocatorRegionSize ||
+          GetCurrentNumberOfAshmemFDs() <
+          0.9 * global_context->ashmem_fd_limit) {
+        int fd;
+        void* address;
+        if (internal::CreateAshmemRegion("", aligned_size, &fd, &address)) {
+          return scoped_ptr<DiscardableMemory>(
+              new DiscardableMemoryAndroidSimple(fd, address, aligned_size));
+        }
+      }
+      return global_context->allocator.Allocate(size);
+    }
+    case DISCARDABLE_MEMORY_EMULATED: {
+      scoped_ptr<internal::DiscardableMemoryEmulated> memory(
+          new internal::DiscardableMemoryEmulated(size));
+      if (!memory->Initialize())

[Philippe, 2013/12/17 14:28:21]

I have to say that I look forward to seeing DiscardableMemoryEmulated being
reused somewhere to justify this pattern being spread across the various
DiscardableMemory implementations :)

[Philippe, 2013/12/17 15:26:22]

Just realized that this may sound harsh. Sorry if it was, I didn't mean it.

[reveman, 2013/12/18 08:12:38]

DiscardableMemoryEmulated is likely not going to be reused. This is for
consistency and make us write code that could be reused if needed.

When it comes to code duplication. I'm mostly concerned with duplication of code
that contains some logic that could potentially be broken. I think this is about
as simple as a one line statement and copying these few lines to a few places
rather than copying a one line statement that calls a static method makes little
difference to me.

[Philippe, 2013/12/18 09:07:47]

Those things are personal preferences obviously and not covered by the style
guide anyway so I'm also fine with it :) I'm just not a big fan of the
constructor + init method pattern when it's not strictly needed (e.g. for code
reuse or when we can't afford the extra heap allocation).
The main reasons in my opinion are the extra code needed on the call site, the
fact that it's easy to miss the Init() call and it's even possible to call
Init() twice.
In addition to that the fact that the object is constructed in two steps
prevents it from being immutable (which is not always possible anyway though).
In my opinion we should not prematurely optimize for code reuse when it implies
extra complexity in the interface. If there is need to reuse at some point then
we can refactor just like you are doing actually. But this is just my opinion
and mostly nitpicking :)

+        return scoped_ptr<DiscardableMemory>();
+
+      return memory.PassAs<DiscardableMemory>();
     }
   }
-  return global_context->allocator.Allocate(size);
+
+  NOTREACHED();
+  return scoped_ptr<DiscardableMemory>();
 }
 
 // static