[tracing] Simplify the MemoryAllocatorDump API

base/trace_event/memory_allocator_dump.h

Index: base/trace_event/memory_allocator_dump.h
diff --git a/base/trace_event/memory_allocator_dump.h b/base/trace_event/memory_allocator_dump.h
index e8d3756bd512e608c3b6ff44b8d1570f12190fa0..71cdad822ba2892cce3315cc9fcc0c7fe675585a 100644
--- a/base/trace_event/memory_allocator_dump.h
+++ b/base/trace_event/memory_allocator_dump.h
@@ -20,58 +20,45 @@ class TracedValue;
 // Data model for user-land memory allocator dumps.
 class BASE_EXPORT MemoryAllocatorDump {
  public:
-  // Returns the absolute name for a given (|allocator_name|,|heap_name|) tuple.
-  static std::string GetAbsoluteName(const std::string& allocator_name,
-                                     const std::string& heap_name);
-
-  // Use as argument for |heap_name| when the allocator has only one root heap.
-  static const char kRootHeap[];
-
   // MemoryAllocatorDump is owned by ProcessMemoryDump.
-  MemoryAllocatorDump(const std::string& allocator_name,
-                      const std::string& heap_name,
+  MemoryAllocatorDump(const std::string& absolute_name,

[petrcermak, 2015/04/28 09:37:09]

Are we calling it "absolute name"? We named the same thing "full name" in Trace
Viewer (which is probably easier to modify).

[Primiano Tucci (use gerrit), 2015/04/28 09:54:18]

Yeah we had some long discussion with Nat about this. absolute is as in
"absolute path" here.

                       ProcessMemoryDump* process_memory_dump);
   ~MemoryAllocatorDump();
 
-  // Name of the allocator, a plain string with no separators (e.g, "malloc").
-  const std::string& allocator_name() const { return allocator_name_; }
-
-  // Name of the heap being dumped, either: "heap", "heap/subheap" or kRootHeap
-  // if the allocator has just one root heap.
-  const std::string& heap_name() const { return heap_name_; }
-
-  // Absolute name, unique within the scope of an entire ProcessMemoryDump.
-  // In practice this is "allocator_name/heap/subheap".
-  std::string GetAbsoluteName() const;
+  // Standard attribute name to model total space requested by the allocator
+  // (e.g., amount of pages requested to the system).
+  static const char kNameOuterSize[];
 
-  // Inner size: Bytes requested by clients of the allocator, without accounting
-  // for any metadata or allocator-specific bookeeping structs.
-  void set_allocated_objects_size_in_bytes(uint64 value) {
-    allocated_objects_size_in_bytes_ = value;
-  }
-  uint64 allocated_objects_size_in_bytes() const {
-    return allocated_objects_size_in_bytes_;
-  }
+  // Standard attribute name to model space for allocated objects, without
+  // taking into account allocator metadata or fragmentation.
+  static const char kNameInnerSize[];
 
-  // Outer size: bytes requested to the system to handle all the allocations,
-  // including any allocator-internal metadata / bookeeping structs. For
-  // instance, in the case of an allocator which gets pages to the system via
-  // mmap() or similar, this is the number of requested pages * 4k.
-  void set_physical_size_in_bytes(uint64 value) {
-    physical_size_in_bytes_ = value;
-  }
-  uint64 physical_size_in_bytes() const { return physical_size_in_bytes_; }
+  // Standard attribute name to model the number of objects allocated.
+  static const char kNameObjectsCount[];
 
-  // Number of objects allocated, if known, or 0 if not available.
-  void set_allocated_objects_count(uint64 value) {
-    allocated_objects_count_ = value;
-  }
-  uint64 allocated_objects_count() const { return allocated_objects_count_; }
+  static const char kTypeScalar[];    // For scalar attriute types.

[petrcermak, 2015/04/28 09:37:09]

nit: I find the comments ("// For XYZ") slightly odd. Why not just say "//
Scalar attribute type."?

[Primiano Tucci (use gerrit), 2015/04/28 09:54:18]

Reworded

+  static const char kTypeString[];    // For string attriute types.
+  static const char kUnitsBytes[];    // For scalar attributes (bytes).
+  static const char kUnitsObjects[];  // For scalar attributes (# objects).

[petrcermak, 2015/04/28 09:37:10]

nit: I don't think there should be a space between "#" and "objects".

[Primiano Tucci (use gerrit), 2015/04/28 09:54:18]

Done.

 
-  // Get/Set extra attributes. The attributes name must have been previously
-  // declared through MemoryDumpProvider.DeclareAllocatorAttribute().
-  void SetAttribute(const std::string& name, int value);
-  int GetIntegerAttribute(const std::string& name) const;
+  // Absolute name, unique within the scope of an entire ProcessMemoryDump.
+  const std::string& absolute_name() const { return absolute_name_; }
+
+  // Generic attribute setter / getter.
+  void Add(const std::string& name,
+           const char* type,
+           const char* units,
+           scoped_ptr<Value> value);
+  bool Get(const std::string& name,

[petrcermak, 2015/04/28 09:37:09]

Do we need a Get method (apart from testing)?

[Primiano Tucci (use gerrit), 2015/04/28 09:54:18]

I need it mainly for testing.
We never know if at some point allocators will need to sneak into what they (or
a parent) dumped.

+           const char** out_type,
+           const char** out_units,
+           const Value** out_value) const;
+
+  // Helper setter for scalar attributes.
+  void AddScalar(const std::string& name, const char* units, uint64 value);

[petrcermak, 2015/04/28 09:37:09]

nit: Even though the signature fits on one line, I would personally break it
apart to look like the other methods.

[Primiano Tucci (use gerrit), 2015/04/28 09:54:18]

Hmm my policy here is "git cl format is always right". If you don't like it file
a git cl format bug
I can fix it, but the next time I'll run git cl format that will be switched
back to this. 

+  void AddString(const std::string& name,
+                 const char* units,
+                 const std::string& value);
 
   // Called at trace generation time to populate the TracedValue.
   void AsValueInto(TracedValue* value) const;
@@ -82,13 +69,9 @@ class BASE_EXPORT MemoryAllocatorDump {
   }
 
  private:
-  const std::string allocator_name_;
-  const std::string heap_name_;
+  const std::string absolute_name_;
   ProcessMemoryDump* const process_memory_dump_;  // Not owned (PMD owns this).
-  uint64 physical_size_in_bytes_;
-  uint64 allocated_objects_count_;
-  uint64 allocated_objects_size_in_bytes_;
-  DictionaryValue attributes_values_;
+  DictionaryValue attributes_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryAllocatorDump);
 };