[Discardable Shared Memory] Adding Windows support.

base/memory/discardable_shared_memory.cc

Index: base/memory/discardable_shared_memory.cc
diff --git a/base/memory/discardable_shared_memory.cc b/base/memory/discardable_shared_memory.cc
index 1b50da260e09b566807fb40e72a1ab0638db11d3..3d03bf375586e0cdaec2d161446839b08c9f0f8e 100644
--- a/base/memory/discardable_shared_memory.cc
+++ b/base/memory/discardable_shared_memory.cc
@@ -21,6 +21,10 @@
 #include "third_party/ashmem/ashmem.h"
 #endif
 
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
 namespace base {
 namespace {
 
@@ -214,6 +218,7 @@ DiscardableSharedMemory::LockResult DiscardableSharedMemory::Lock(
   DCHECK_EQ(locked_pages_.size(), locked_page_count_);
 #endif
 
+// Pin pages if supported.
 #if defined(OS_ANDROID)
   SharedMemoryHandle handle = shared_memory_.handle();
   if (SharedMemory::IsHandleValid(handle)) {
@@ -222,6 +227,14 @@ DiscardableSharedMemory::LockResult DiscardableSharedMemory::Lock(
       return PURGED;
     }
   }
+#elif defined(OS_WIN)
+  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {

[jschuh, 2015/11/03 05:11:24]

Save the version off into a static, otherwise you're traversing the singleton
pointers on every call.

[penny, 2015/11/03 18:25:44]

In upload #1 I was doing a lazy init of a static variable for this.  However,
reveman thought that since OSInfo acts as a singleton and caches its own OS
version, it would fast and cleaner to just access that.

Further thoughts?

[brucedawson, 2015/11/04 21:51:27]

Both options are quite cheap, with the exact cost depending on whether LTCG
(buildtype=Official only) is in a good mood. Calling GetVersion() each time
requires (in the generated code) testing a pointer and then dereferencing it,
with possible some function calls in there. Adding a static requires testing a
flag and then returning the cached value.

Calling GetVersion() is more likely to touch more cache lines and make actual
function calls, whereas a static means introducing a couple more globals. If
LTCG aggressively inlines GetInstance() then this makes the static option
wasteful since that code will rarely be used.

It's practically a wash and I doubt this is called enough to matter, but I'd
lean towards the static. Examining the machine code to see what actually happens
in official builds is not worth it.

[jschuh, 2015/11/04 21:52:43]

Honestly, I prefer the other but I don't feel strongly either way and can't say
authoritatively which is better.

[penny, 2015/11/05 18:45:19]

Thanks Bruce and Justin for input here.  I don't feel emotional about this
either.  I've gone back to a static class variable, but I've made things cleaner
than my first upload.  The lazy init happens inline in Lock or Unlock now, with
now extra function call in there.

David, let me know if you're happier with this new version as well!

+    if (!VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
+                          AlignToPageSize(sizeof(SharedState)) + offset,
+                      length, MEM_RESET_UNDO, PAGE_READWRITE)) {
+      return PURGED;
+    }
+  }
 #endif
 
   return SUCCESS;
@@ -240,6 +253,7 @@ void DiscardableSharedMemory::Unlock(size_t offset, size_t length) {
 
   DCHECK(shared_memory_.memory());
 
+// Unpin pages if supported.
 #if defined(OS_ANDROID)
   SharedMemoryHandle handle = shared_memory_.handle();
   if (SharedMemory::IsHandleValid(handle)) {
@@ -248,7 +262,15 @@ void DiscardableSharedMemory::Unlock(size_t offset, size_t length) {
       DPLOG(ERROR) << "ashmem_unpin_region() failed";
     }
   }
+#elif defined(OS_WIN)
+  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {

[jschuh, 2015/11/03 05:11:24]

This shouldn't be version gated. It's supported on all Windows versions as far
as we're concerned.

[penny, 2015/11/03 18:25:44]

So this is in the Unlock function, and needs to match the Lock function above
for pinning.  Since MEM_RESET_UNDO is only supported on >= Win8 for the Lock, we
can only do the RESET on >= Win8 as well.

[jschuh, 2015/11/04 21:52:43]

Fair enough, this should be biased to preserving the contents of memory. Could
we get a comment somewhere to that effect, because in reading the code I assumed
it should be biased to releasing memory.

[penny, 2015/11/05 18:45:20]

I've updated the comments to try to provide clarity.

+    if (!VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
+                          AlignToPageSize(sizeof(SharedState)) + offset,
+                      length, MEM_RESET, PAGE_READWRITE)) {
+      DPLOG(ERROR) << "VirtualAlloc() MEM_RESET failed in Unlock()";
+    }
 #endif
+}

[jschuh, 2015/11/03 05:11:24]

Looks like the indentation is wrong and it should be inside the #if block. I'm
guessing this is why the compile is failing.

[penny, 2015/11/03 18:25:44]

Done.

 
   size_t start = offset / base::GetPageSize();
   size_t end = start + length / base::GetPageSize();
@@ -340,6 +362,14 @@ bool DiscardableSharedMemory::Purge(Time current_time) {
               AlignToPageSize(mapped_size_), MADV_PURGE_ARGUMENT)) {
     DPLOG(ERROR) << "madvise() failed";
   }
+#elif defined(OS_WIN)
+  // MEM_DECOMMIT the purged pages to release the physical storage,
+  // either in memory or in the paging file on disk.  Pages remain RESERVED.
+  if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) +

[jschuh, 2015/11/03 05:11:24]

This doesn't look like what you want -- it's basically equivalent to POSIX
mprotect(PROT_NONE). The correct approach would be some combination of the
following:
* DiscardVirtualMemory() - fast, but isn't guaranteed to zero-fill,
  requires Win8+, and is flaky on Win10, so you still have to fallback
  to one of the following
* MEM_DECOMMIT followed by MEM_COMMIT - Two syscalls, so a bit slower but
  will guarantee zero-fill
* MEM_RESET - One syscall, but releases the memory lazily and doesn't
  guarantee zero-fill

[penny, 2015/11/03 18:25:44]

So a few notes:
- We don't need to guarantee any sort of zero fill on purge for the functioning
of DSM.
- This memory won't be used again, it'll just be asynchronously freed "later".
- This is the Purge function, so we're trying to do the most releasing of
resources that we can here (this function is called synchronously from the
browser process as soon as there is a resource problem).  We can't do a full
free at this point, until the child/renderer process notices that there has been
a purge and it can release its own references.  This is my understanding from
what reveman has told me, but I'm happy to get clarification from him on that. 
- I was originally going to just MEM_RESET here, but given the above point, it's
not clear why I wouldn't just DECOMMIT to do an immediate release of resource. 
Also, on >= win8, an unlocked segment that would be a candidate for purge will
already be in MEM_RESET state (from the last Unlock).  That's not a deal-breaker
or anything, but it would be nice to do an immediate release on purge.

[brucedawson, 2015/11/04 21:51:27]

MEM_DECOMMIT will indeed free up the pages, making them fully available to other
processes or the file cache. This seems like absolutely the right thing to do
when we are under memory pressure. MEM_RESET seems appropriate for if we want
the OS to decide whether or not it needs the memory, and that doesn't sound like
the case here.

I don't understand this:
I understand the first part, but not the second. MEM_DECOMMIT will free the
pages immediately.

FWIW, the cost of doing a MEM_DECOMMIT and then a MEM_COMMIT of the same address
range is *not* the cost of the two system calls - those are very cheap. The cost
is the time to remove the pages from the memory map, zero them, and then fault
them back in. So, we should only MEM_DECOMMIT memory that we don't think we'll
need for a bit. Tricky, I know.

Now, the cost of MEM_RESET where the memory is trimmed from the working set and
then brought back in later should be comparable. However if we are wrong and the
pages are reused by Chrome shortly afterwards then MEM_RESET will be faster
because the memory mapping doesn't get changed.

I analyze and measured these hidden costs last year and blogged about them:
https://randomascii.wordpress.com/2014/12/10/hidden-costs-of-memory-allocation/
Actual timing results depend significantly on your CPU and OS, I think.

[jschuh, 2015/11/04 21:52:43]

Okay, please tweak the comment above on the OS_POSIX section so it clearly
explains that we're not using this address range anymore, but using madvise()
this way because it's faster.

[penny, 2015/11/05 18:45:20]

I've added comments for this section, providing more context - without
mentioning "browser" vs "renderer" in base\.

+                       AlignToPageSize(sizeof(SharedState)),
+                   AlignToPageSize(mapped_size_), MEM_DECOMMIT)) {
+    DPLOG(ERROR) << "VirtualFree() MEM_DECOMMIT failed in Purge()";
+  }
 #endif
 
   last_known_usage_ = Time();