Support persistent system profiles.

components/metrics/persistent_system_profile.h

Index: components/metrics/persistent_system_profile.h
diff --git a/components/metrics/persistent_system_profile.h b/components/metrics/persistent_system_profile.h
new file mode 100644
index 0000000000000000000000000000000000000000..526ee3743c1d6c6c16f9dbfd957ca087a47196c1
--- /dev/null
+++ b/components/metrics/persistent_system_profile.h
@@ -0,0 +1,102 @@
+// Copyright 2017 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.
+
+#ifndef BASE_METRICS_PERSISTENT_SYSTEM_PROFILE_H_
+#define BASE_METRICS_PERSISTENT_SYSTEM_PROFILE_H_
+
+#include <vector>
+
+#include "base/threading/thread_checker.h"
+#include "components/metrics/proto/system_profile.pb.h"
+
+namespace base {
+

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

Nit: No need for inner newlines for forward declarations.

[bcwhite, 2017/05/29 18:32:26]

Done.

+class PersistentMemoryAllocator;
+
+}  // namespace base
+
+namespace metrics {
+
+// Manage a copy of the system profile inside persistent memory segments.

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

Nit: Manages

[bcwhite, 2017/05/29 18:32:26]

Done.

+class PersistentSystemProfile {
+ public:
+  PersistentSystemProfile();
+  ~PersistentSystemProfile();
+
+  // This object can store records in multiple memory allocators.
+  void RegisterPersistentAllocator(
+      base::PersistentMemoryAllocator* memory_allocator);
+  void DeregisterPersistentAllocator(
+      base::PersistentMemoryAllocator* memory_allocator);
+
+  // Stores a complete system profile.
+  void SetSystemProfile(const SystemProfileProto& system_profile);
+
+  // Retrieves the system profile from a persistent memory allocator. Returns
+  // true if a profile was successfully retrieved.
+  static bool GetSystemProfile(
+      SystemProfileProto* system_profile,
+      const base::PersistentMemoryAllocator* memory_allocator);

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

If the param can't be null, pass by const ref instead of pointer.

Also, swap the param order, since system_profile is an out param.

[bcwhite, 2017/05/29 18:32:26]

For historical reasons, the persistent allocator is always passed as a pointer.

[Alexei Svitkine (slow), 2017/05/29 19:48:32]

I don't see why it needs to be that way. The style guide says to pass things by
const ref unless there's a case they can be null. So please do that for new
code. If you have a ptr in the caller, you can just put a * before it to pass
here.

Also, please change the order of the params as well, since system_profile param
will be changed.

[bcwhite, 2017/05/29 20:56:35]

Done.

+
+ private:
+  friend class PersistentSystemProfileTest;
+
+  // Defines record types that can be stored inside our local Allocators.
+  enum RecordType : uint8_t {
+    kUnusedSpace,
+    kPadding,

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

This seems unused?

[bcwhite, 2017/05/29 18:32:26]

I was expecting to need it when more record types get implemented should there
be a little space at the end of a segment but I think it's now unnecessary. 
Removed.

+    kSystemProfileProto,
+  };
+
+  // A class for managing record allocations inside a persistent memory segment.
+  class RecordAllocator {
+   public:
+    // Construct an allocator for writing.
+    RecordAllocator(base::PersistentMemoryAllocator* memory_allocator,
+                    size_t min_size);
+
+    // Construct an allocator for reading.
+    RecordAllocator(const base::PersistentMemoryAllocator* memory_allocator);
+
+    bool Matches(const base::PersistentMemoryAllocator* allocator) const;

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

Nit: Add a comment.

Alternatively, how about just having an accessor for allocator() and then the
caller could do the comparison themselves?

[bcwhite, 2017/05/29 18:32:26]

Done.

+
+    // These methods manage writing records to the allocator. Do not mix these
+    // with "read" calls; it's one or the other.
+    void Reset();

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

Given you always call Reset() before Write(), how about combining the two? (i.e.
Reset() being an implementation detail of Write().

[bcwhite, 2017/05/29 18:32:26]

Once there are more record types than just the system profile, that will no
longer be the case.

+    bool Write(RecordType type, const std::string& record);
+
+    // Read a record from the allocator. Do not mix this with "write" calls;
+    // it's one or the other.
+    bool Read(RecordType* type, std::string* record) const;
+
+   private:
+    // Advance to the next record segment in the memory allocator.
+    bool NextSegment() const;
+
+    // Advance to the next record segment, creating a new one if necessary with
+    // sufficent |min_size| space.
+    bool AddSegment(size_t min_size);
+
+    // This never changes but can't be "const" because vector calls operator=().
+    base::PersistentMemoryAllocator* allocator_;  // Storage location.
+
+    // These change even though the underlying data may be "const".
+    mutable uint32_t tail_reference_;  // Last storage block.
+    mutable uint32_t end_offset_;      // End of data in block.

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

This field is not clear to me.

Why is it an offset, and not something like filled_size_?

[bcwhite, 2017/05/29 18:32:26]

It follows the STL begin/end naming.  It's not strictly the "filled" size
because the reading process uses the same offset to mark its place.

+
+    // Copy and assign are allowed for easy use with STL containers.
+  };
+
+  // The list of registered persistent allocators, described by RecordAllocator
+  // instances.
+  std::vector<RecordAllocator> allocators_;
+
+  THREAD_CHECKER(thread_checker_);

[Alexei Svitkine (slow), 2017/05/26 18:01:53]

Nit: Make this the first member.

[bcwhite, 2017/05/29 18:32:26]

Typically its the last member.

[Alexei Svitkine (slow), 2017/05/29 19:48:32]

That's not been my experience back with ThreadChecker (before the macro was
added). I don't thinking making it the last member makes sense because a) it's
not required and b) WeakPtrFactory needs to be the last member. So for classes
using both of these, it's not possible for both of them to be last member. So it
seems better to have it be the first member.

[bcwhite, 2017/05/29 20:56:35]

I checked a 1/2 dozen files before replying and all but one were at the end. 
The other was somewhere in the middle.

And I'm pretty sure I had a review comment once specifically telling me to put
it at the end.

+
+  DISALLOW_COPY_AND_ASSIGN(PersistentSystemProfile);
+};
+
+}  // namespace metrics
+
+#endif  // BASE_METRICS_PERSISTENT_SYSTEM_PROFILE_H_