base: Discardable memory types.

base/memory/discardable_memory_android.cc

 // Copyright (c) 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_android.h"
 
 #include <sys/mman.h>
 #include <sys/resource.h>
 #include <sys/time.h>
 #include <unistd.h>
 
 #include <limits>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #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"
 
 namespace base {
 namespace {
 
 const size_t kPageSize = 4096;
 
 const char kAshmemAllocatorName[] = "DiscardableMemoryAllocator";
 
@@ skipping 80 matching lines @@
   AutoLock lock(g_context.Get().lock);
   return g_context.Get().ashmem_fd_count();
 }
 
 // Returns whether the provided size can be safely page-aligned (without causing
 // an overflow).
 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 {
 
 size_t AlignToNextPage(size_t size) {
   DCHECK_EQ(static_cast<int>(kPageSize), getpagesize());
   DCHECK(CheckSizeCanBeAlignedToNextPage(size));
   const size_t mask = ~(kPageSize - 1);
   return (size + kPageSize - 1) & mask;
 }
@@ skipping 61 matching lines @@
   if (failed)
     DLOG(ERROR) << "Failed to unpin memory.";
   // This allows us to catch accesses to unlocked memory.
   DCHECK_EQ(0, mprotect(address, size, PROT_NONE));
   return !failed;
 }
 
 }  // 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:
 // - Each client-requested allocation is backed by an individual ashmem region.
 // This allows deleting ashmem regions individually by closing the ashmem file
 // descriptor. This is the default path that is taken when file descriptor usage
 // allows us to do so or when the allocation size would require and entire
 // ashmem region.
 // - Allocations are performed by the global allocator when file descriptor
 // usage gets too high. This still allows unpinning but does not allow deleting
 // (i.e. releasing the physical pages backing) individual regions.
 //
 // TODO(pliard): consider tuning the size threshold used below. For instance we
 // might want to make it a fraction of kMinAshmemRegionSize and also
 // systematically have small allocations go through the allocator to let big
 // allocations systematically go through individual ashmem regions.
 //
 // 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
 bool DiscardableMemory::PurgeForTestingSupported() {
   return false;
 }
 
 // static
 void DiscardableMemory::PurgeForTesting() {
   NOTIMPLEMENTED();
 }
 
 }  // namespace base