Harden allocator for file-backed memory.

base/metrics/persistent_memory_allocator.h

Index: base/metrics/persistent_memory_allocator.h
diff --git a/base/metrics/persistent_memory_allocator.h b/base/metrics/persistent_memory_allocator.h
index e0ac867578ab47def39781c66c8b3bcc7a8c2f85..c7a914ea762ca0b82c15db740dfc1470cb3793c4 100644
--- a/base/metrics/persistent_memory_allocator.h
+++ b/base/metrics/persistent_memory_allocator.h
@@ -96,6 +96,20 @@ class BASE_EXPORT PersistentMemoryAllocator {
  public:
   typedef uint32_t Reference;
 
+  // These states are used to indicate the overall condition of the memory
+  // segment irrespective of what is stored within it. Because the data is
+  // often persistent and thus needs to be readable by different versions of
+  // a program, these values are fixed and can never change.
+  enum : uint8_t {
+    MEMORY_UNINITIALIZED = 0,  // Persistent memory starts all zeros.
+    MEMORY_INITIALIZED = 1,    // The header has been written.
+    MEMORY_DELETED = 2,        // The data should be considered deleted.

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

Can you define what "considered deleted" means?

[bcwhite, 2017/03/15 19:21:48]

Done.

+
+    // Outside code can create states starting with this number; these too
+    // must also never change between code versions.
+    MEMORY_USER_DEFINED = 100,
+  };
+
   // Iterator for going through all iterable memory records in an allocator.
   // Like the allocator itself, iterators are lock-free and thread-secure.
   // That means that multiple threads can share an iterator and the same
@@ -279,6 +293,10 @@ class BASE_EXPORT PersistentMemoryAllocator {
   // Is this segment open only for read?
   bool IsReadonly() { return readonly_; }
 
+  // Manage the saved state of the memory.
+  void SetMemoryState(uint8_t memory_state);
+  uint8_t GetMemoryState();
+
   // Create internal histograms for tracking memory use and allocation sizes
   // for allocator of |name| (which can simply be the result of Name()). This
   // is done seperately from construction for situations such as when the
@@ -290,6 +308,17 @@ class BASE_EXPORT PersistentMemoryAllocator {
   //    UMA.PersistentAllocator.name.UsedPct
   void CreateTrackingHistograms(base::StringPiece name);
 
+  // Flushes the persistent memory to any backing store. This typically does
+  // nothing but is used by the FilePersistentMemoryAllocator to inform the
+  // OS that all the data should be sent to the disk immediately. This is
+  // useful in the rare case where something has just been stored that needs
+  // to survive a hard shutdown of the machine like from a power failure.
+  // The |sync| parameter indicates if this call should block until the flush
+  // is complete but is only advisory and may or may not have an effect
+  // depending on the capabilities of the OS. Synchronous flushes are allowed
+  // only from theads that are allowed to do I/O.
+  void Flush(bool sync) { Flush(used(), sync); }

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

Don't overload a protected function with a public one. How about naming the
internal one FlushInternal()?

Also, this doesn't look like meeting the criteria for inline function from the
style guide:

"Inline functions

Simple accessors should generally be the only inline functions. These should be
named unix_hacker_style(). Virtual functions should never be declared this way.
For more detail, consult the C++ Dos and Don'ts section on inlining."

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[bcwhite, 2017/03/15 19:21:48]

Done.  At some point it may be useful to become FlushRange with an additional
|start| parameter but right now that would just be wasted.

[Alexei Svitkine (slow), 2017/03/15 20:05:29]

The second part of this comment was about not having it inline "Simple accessors
should generally be the only inline functions". Move it out of line. It's not
like it should be called often, so being out of line shouldn't be a problem.

[bcwhite, 2017/03/16 15:53:03]

Done.  Originally it was just providing a default parameter which is, as I
understand it, allowed by the style.

+
   // Direct access to underlying memory segment. If the segment is shared
   // across threads or processes, reading data through these values does
   // not guarantee consistency. Use with care. Do not write.
@@ -576,6 +605,9 @@ class BASE_EXPORT PersistentMemoryAllocator {
                             uint64_t id, base::StringPiece name,
                             bool readonly);
 
+  // Implementation of Flush that accepts how much to flush.
+  virtual void Flush(size_t length, bool sync);
+
   volatile char* const mem_base_;  // Memory base. (char so sizeof guaranteed 1)
   const MemoryType mem_type_;      // Type of memory allocation.
   const uint32_t mem_size_;        // Size of entire memory segment.
@@ -711,6 +743,10 @@ class BASE_EXPORT FilePersistentMemoryAllocator
   // the rest.
   static bool IsFileAcceptable(const MemoryMappedFile& file, bool read_only);
 
+ protected:
+  // PersistentMemoryAllocator:
+  void Flush(size_t length, bool sync) override;
+
  private:
   std::unique_ptr<MemoryMappedFile> mapped_file_;