[Tracing] Introduce HeapDumpWriter helper class

base/trace_event/memory_profiler_heap_dumper.h

Index: base/trace_event/memory_profiler_heap_dumper.h
diff --git a/base/trace_event/memory_profiler_heap_dumper.h b/base/trace_event/memory_profiler_heap_dumper.h
new file mode 100644
index 0000000000000000000000000000000000000000..ac451328e617c326271c2a6867e76d0ac6c091d9
--- /dev/null
+++ b/base/trace_event/memory_profiler_heap_dumper.h
@@ -0,0 +1,67 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

I'd probably pick a stronger name for this, which explains what it does.
HeapDumper IMHO is a bit generic and might sound as "something which collects
and dumps the heap", while this is specifically for the dumping part.
How about HeapDumpSeralizer or something like that?

[Ruud van Asseldonk, 2015/10/26 14:51:26]

Renamed to |HeapDumpWriter|.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TRACE_EVENT_MEMORY_PROFILER_HEAP_DUMPER_H_
+#define BASE_TRACE_EVENT_MEMORY_PROFILER_HEAP_DUMPER_H_
+
+#include <string>
+
+#include "base/base_export.h"
+#include "base/containers/hash_tables.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/trace_event/memory_profiler_allocation_context.h"
+#include "base/trace_event/trace_event_argument.h"
+
+namespace base {
+namespace trace_event {
+
+class AllocationRegister;
+
+// Helper class to dump a snapshot of an |AllocationRegister| into a
+// |TracedValue|. This class is intended to be used as a one-shot local
+// instance on the stack. To write heap dumps, call |Fill| on it, then
+// call |WriteHeapDump|.
+class BASE_EXPORT HeapDumper {
+ public:
+  // The |StackFrameDeduplicator| is borrowed. The heap dumper assumes unique
+  // access to it during dumper lifetime.
+  HeapDumper(scoped_refptr<TracedValue> traced_value,

[Primiano Tucci (use gerrit), 2015/10/26 11:52:07]

This feels a bit an unnatural pattern: you take an empty TracedValue at ctor
time, and you don't do anything with that until you call the WriteHeapDump
method.
Since TracedValue can be created everywhere and are easily movable, shouldn't
you don't take any traced value and either:
- Create one in the WriteHeapDump function and return it out.
- Create one as a private field in this class and return it from WriteHeapDump()
?

The current signatures are a bit cryptic

[Ruud van Asseldonk, 2015/10/26 14:51:26]

Made it a member instead, and return it from |WriteHeapDump|. (It is nice to
have a member for |WriteSize|.)

+             StackFrameDeduplicator* sf_deduplicator);

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

either deduplicator or stack_frame_deduplicator

[Ruud van Asseldonk, 2015/10/26 14:51:26]

Done.

+  ~HeapDumper();
+
+  // Extracts all required information from the allocation register. This does
+  // minimal processing, so if the caller holds a lock for |allocation_register|
+  // the lock can be released as soon as possible.
+  void Fill(const AllocationRegister& allocation_register);

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

As discussed offline would be clearer to expose only a AddAllocation(Context,
size) method here and do the loop outside.
In this way you make clear that this class is orthogonal to the fact that the
allocator uses or not the AllocationRegister

[Ruud van Asseldonk, 2015/10/26 14:51:26]

I agree that would be much cleaner. Done.

+
+  // Aggregates allocations and writes a "heap" array to the traced value. Call
+  // |Fill| before using this.
+  void WriteHeapDump();
+
+ private:
+  // Writes a "size" key with value |size| as a hexidecimal string to the traced
+  // value.
+  void WriteSize(size_t size);
+
+  // The value that this heap dumper writes to.
+  const scoped_refptr<TracedValue> traced_value_;
+
+  // Helper for generating the |stackFrames| dictionary. Borrowed, not owned.

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

Borrowed doesn't make clear if you plan to transfer ownership (in which case you
should use a scoped_ptr or not).
I'd say "Not owned. the caller is resposible to keep the SFD instance alive for
the lifetime of this class"

[Ruud van Asseldonk, 2015/10/26 14:51:26]

Done.

I don't know about you, but to me "borrowing" implies the intention to return it
:) So that would mean no transfer of ownership. (In Rust a borrowed pointer is a
thing.)

+  StackFrameDeduplicator* const sf_deduplicator_;
+
+  // A map of backtrace to the number of bytes allocated for that backtrace.
+  hash_map<Backtrace, size_t, BacktraceHash> acc_by_backtrace_;

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

what about bytes_by_backtrace_?

[Ruud van Asseldonk, 2015/10/26 14:51:27]

If you wish.

+
+  // Buffer for converting integers into strings, that is re-used throughout the
+  // dump.
+  std::string buffer_;

[Primiano Tucci (use gerrit), 2015/10/26 11:52:06]

smart, I see what you are doing here  ;-)

[Ruud van Asseldonk, 2015/10/26 14:51:26]

The credit is yours, I stole this from
|MemoryAllocatorDump::string_conversion_buffer_| :)

+
+  DISALLOW_COPY_AND_ASSIGN(HeapDumper);
+};
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_PROFILER_HEAP_DUMPER_H_