Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

Issue 2650863003: New heap dump format.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 6 days ago by DmitrySkiba
Modified:
2 days, 8 hours ago
Reviewers:
Primiano Tucci
CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), vmpstr+watch_chromium.org, wfh+watch_chromium.org, haraken, blink-reviews, tracing+reviews_chromium.org, kinuko+watch, kouhei+heap_chromium.org, lpy
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

New heap dump format. TODO: unittests BUG=

Patch Set 1 #

Patch Set 2 : Add 'version' field. #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -570 lines) Patch
M base/trace_event/heap_profiler_heap_dump_writer.h View 1 chunk +2 lines, -87 lines 0 comments Download
M base/trace_event/heap_profiler_heap_dump_writer.cc View 1 chunk +56 lines, -286 lines 3 comments Download
M base/trace_event/heap_profiler_heap_dump_writer_unittest.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M base/trace_event/heap_profiler_stack_frame_deduplicator.h View 4 chunks +9 lines, -7 lines 0 comments Download
M base/trace_event/heap_profiler_stack_frame_deduplicator.cc View 2 chunks +15 lines, -31 lines 1 comment Download
M base/trace_event/heap_profiler_type_name_deduplicator.h View 1 chunk +11 lines, -7 lines 1 comment Download
M base/trace_event/heap_profiler_type_name_deduplicator.cc View 3 chunks +11 lines, -24 lines 0 comments Download
M base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M base/trace_event/malloc_dump_provider.cc View 1 chunk +4 lines, -16 lines 0 comments Download
M base/trace_event/memory_dump_manager.cc View 1 3 chunks +49 lines, -39 lines 2 comments Download
M base/trace_event/memory_dump_manager_unittest.cc View 3 chunks +72 lines, -0 lines 1 comment Download
M base/trace_event/process_memory_dump.h View 2 chunks +2 lines, -4 lines 0 comments Download
M base/trace_event/process_memory_dump.cc View 3 chunks +8 lines, -13 lines 0 comments Download
M base/trace_event/process_memory_dump_unittest.cc View 3 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp View 1 chunk +2 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp View 1 chunk +2 lines, -18 lines 0 comments Download
M third_party/WebKit/Source/platform/instrumentation/tracing/web_process_memory_dump.h View 2 chunks +0 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/instrumentation/tracing/web_process_memory_dump.cc View 1 chunk +0 lines, -10 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 6 (2 generated)
DmitrySkiba
2 weeks, 6 days ago (2017-01-30 19:44:11 UTC) #2
DmitrySkiba
PTAL
5 days, 7 hours ago (2017-02-14 18:31:15 UTC) #4
Primiano Tucci
On 2017/02/14 18:31:15, DmitrySkiba wrote: > PTAL sorry for the delay, manage to do *almost* ...
4 days, 6 hours ago (2017-02-15 18:55:02 UTC) #5
Primiano Tucci
2 days, 8 hours ago (2017-02-17 17:07:05 UTC) #6
Thanks  a lot for this. I'm very excited by the code being ditched here.
Everything looks great, I have only some minor comments.

The other doubt that I have is that the incremental approach, right now, will
break the heap profiler in ring buffer mode.
At least we shoudl add a CHECK somewhere saying: "sorry, cant use heap profiler
in ring buffer mode"

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:44: hash_map<AggregationKey,
AllocationMetrics, AggregationKey::Hasher>
note: at some point we should fix the deduplicators as per discussion in
crbug.com/686208 .
I'd do as a separate CL to keep this easier to review. I think the only think we
need is a further hashtable in each deduplicator that maps (const char*) ->
(string hash). That will still be extremely fast and deal with string identity.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:46: for (const auto&
allocation : allocation_register) {
would be very nice if here there was a comment with a rough summary of the heap
dump format and a link to the doc.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:64: for (const auto&
key_and_metrics : metrics_by_key) {
I think the rest of the code in this folder doesn't have braces when unnecessary
(e.g. single line for-loops) I find it helps to keep files more compact and
easier to glance and scroll

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:99:
traced_value->SetInteger("parent", frame_node.parent_frame_index);
do we have the guarantee that the parent is always emitted (or has been emitted
by the previous snapshots)?

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_type_name_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_type_name_deduplicator.h:41:
std::vector<TypeMap::const_iterator> new_type_ids_;
smart ;-)

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:725:
heap_dump_value->SetInteger("version", 1);
I think that historically we avoided any sort of versioning in tracing. The deal
is that when you change something is either:
- incremental changes that can still be parsed by the old UI
- you change the name of the key so that old version of the UI will ignore this
completely.

The problem with the version approach, is that we definitely run into cases
where the UI is older than the trace format. So the day that you will introduce
version "2" here, there will be people out there running a version of the UI
that doesn't know about the existance of version 2 and will still try to parse
this as version 1.
As such let's remove the versioning  and keep using the same pattern

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:753:
TraceLog::GetCategoryGroupEnabled(kTraceCategory), "heap_profile",
this is the only thing where I'm not fully sure. Why do we want to emit another
dump type if, at the end, this is generated synchronously with the memory-infra
dump and there are dependencies between the two?
I don't think this will make that much of a difference in terms of code reusing
in the importer.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1293: class HeapDumpProvider :
public MemoryDumpProvider {
shouldn't these be in the *heap_profiler_something*unittest ?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd