Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494293005:
[Tracing] Enable heap dumps with both type info and backtraces (Closed)
|
Created:
5 years ago by Ruud van Asseldonk Modified:
5 years ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, darin-cc_chromium.org, jam, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@sfdedup-iter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Tracing] Enable heap dumps with both type info and backtraces
This is a major change to |HeapDumpWriter| which enables it to dump the
format described in https://goo.gl/KY7zVE. This will allow the heap
profiler to break down the heap by type or backtrace, but also
combinations of those.
This changes the interface of |HeapDumpWriter| slightly from a two-step
process (insert allocations, dump json) to a three-step process (insert
allocations, compute what to dump, convert to json). The main reason for
doing this is testability. Aggregation and json conversion are now
completely orthogonal. This means that the aggregation algorithm can be
tested without having to parse the json first, and the json conversion
is simpler to test too.
This is part of the heap profiler in chrome://tracing.
BUG=524631
Committed: https://crrev.com/3314e2e367138427f3aaccd3da030413c045ab56
Cr-Commit-Position: refs/heads/master@{#364444}
Patch Set 1 #
Total comments: 2
Patch Set 2 : 32-bit is still a thing #
Total comments: 36
Patch Set 3 : Address petrcermak comments #Patch Set 4 : Avoid reuse and generic code, opt for less helper structs instead #
Total comments: 28
Patch Set 5 : Address most primiano comments #
Total comments: 12
Patch Set 6 : Alternative to patchset 5 #Patch Set 7 : IWYU #
Total comments: 25
Patch Set 8 : Address primiano comments #Patch Set 9 : Rename _all_ occurrences #
Messages
Total messages: 20 (5 generated)
ruuda@google.com changed reviewers: + petrcermak@chromium.org, primiano@chromium.org
This changes the meaning of the the dumped values, so it should not land until trace viewer can deal with that. +petrcermak, adding you to make sure we are on the same page about the format. https://codereview.chromium.org/1494293005/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_heap_dump_writer.cc:77: using Property = Slice Part::*; I really did try to avoid this, but being able to write std::vector<Bucket> by_backtrace = BreakDownBy(bucket, &Part::backtrace); std::vector<Bucket> by_type_name = BreakDownBy(bucket, &Part::type_name); was just too neat not to give it a try. As a sacrifice to please the underappreciated-c++-feature gods, I slaughtered all of the templates in this file. If you think this is too esoteric, there are a few alternatives: * Use a function pointer with signature |Slice* (const Part&)|. Though arguably the function pointer syntax is just as abstruse as the pointer-to-member syntax. * Have an enum for the properties and put switch statements all over the place. https://codereview.chromium.org/1494293005/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_heap_dump_writer.cc:190: buckets.erase(buckets.begin(), it); This is slightly inefficient. The elements that I need are at the end of the vector, so this will move everything. It is unnecessary, because all I want to do is iterate the elements. I opted for simplicity here. Ideally we would pass the unmodified vector and |it|, so |BreakDown| would start iterating at the right point, but that would require some boilerplate to wrap the vector. Alternatively, we could use a deque instead of a vector, but a deque does not have |reserve| and would cause more allocations.
Looks really good overall! Here's just a couple of minor comments. Thanks, Petr https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:21: // Most of what the |HeapDumpWriter| does is aggregating the detailed nit: I don't think there should be a definite article in front of "detailed" here https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:23: // process is a list of (AllocationContext, size). The context has several nit: I'd say "tuple" before the full stop. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:23: // process is a list of (AllocationContext, size). The context has several supernit: s/several/two/ (I'd expect "several" to mean more than 2). Feel free to ignore if you have a different perception about "several" :-) https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:42: // is a group of parts of which the properties share a prefix. This is quite cryptic. Could you please provide an explicit example of |Part|s and |Bucket|s here? https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:42: // is a group of parts of which the properties share a prefix. nit: s/of which the/whose/ https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:76: // Pointer to member of |Part| that has type |Slice|. nit: "a member" https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:79: // Helper for breaking down the heap. The slices reference an allocation Do multiple slices correspond to one allocation context? If not, you should say "Each slice references an allocation context..." https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:81: // allocated for this context. The |cursor| of the slices indicates the current If the previous comment applies, go to page 200 ... just joking ... in that case, s/slices/slice/ https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:82: // position in the tree when breaking down. The branches taken are [begin, "branches taken" is difficult to understand. What about "the patch taken in the tree so far is [begin, cursor)"? https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:106: return Part(property == &Part::backtrace ? backtrace.next() : backtrace, Thought: Is it worth keeping copying backtrace/type_name over using smart pointers here? https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:152: return buckets; Are move semantics already allowed in Chrome? If they are, this might be a good place to use them (if I'm not mistaken). https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:192: return buckets; ditto https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:220: // If the type name is not set use ID -1, otherwise deduplicate the type name. supernit: I'd say "ID = -1" (multiple uses in this patch). Feel free to ignore. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:31: // the processing, and pass it to |Write| to get a |TracedValue| that can be nit: I'd say "... and pass its return value to |Write| ...". https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:62: // in the "entries" array. Use |Write| to convert to a traced value. It would probably be worth saying here that this also decides which values are "important" and which are not. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:78: // A map of allocation context to the number of bytes allocated for that nit: s/map of/map from/ https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:53: // Note: Using Gtest |ASSERT_TRUE| instead of |DCHECK| causes a compile error. Not sure this is a good idea because DCHECK won't run on a Release build (unless you compile it with a special flag). https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:85: TEST(HeapDumpWriterTest, EmptyBacktraceIndexIsEmptyString) { I don't think the test name matches what the test does. You also test a non-empty backtrace. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:104: TEST(HeapDumpWriterTest, CumulativeTypeEntryOmitsTypeId) { ditto
Thank you for the detailed feedback. Please take another look. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:21: // Most of what the |HeapDumpWriter| does is aggregating the detailed On 2015/12/04 19:13:20, petrcermak wrote: > nit: I don't think there should be a definite article in front of "detailed" > here As you wish. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:23: // process is a list of (AllocationContext, size). The context has several On 2015/12/04 19:13:20, petrcermak wrote: > supernit: s/several/two/ (I'd expect "several" to mean more than 2). Feel free > to ignore if you have a different perception about "several" :-) Yes only two is a bit of an anticlimax here after mentioning “several”, I agree. But there might be more in the future. Changed to “two”. On 2015/12/04 19:13:20, petrcermak wrote: > nit: I'd say "tuple" before the full stop. That might be confusing with |std::tuple|. Spelled it out instead. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:42: // is a group of parts of which the properties share a prefix. On 2015/12/04 19:13:20, petrcermak wrote: > This is quite cryptic. Could you please provide an explicit example of |Part|s > and |Bucket|s here? I tried an example, but even a very minimal example will turn into a two-page comment quickly. I elaborated a bit more, please take another look. On 2015/12/04 19:13:20, petrcermak wrote: > nit: s/of which the/whose/ No, whose refers to humans only. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:76: // Pointer to member of |Part| that has type |Slice|. On 2015/12/04 19:13:20, petrcermak wrote: > nit: "a member" Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:79: // Helper for breaking down the heap. The slices reference an allocation On 2015/12/04 19:13:20, petrcermak wrote: > Do multiple slices correspond to one allocation context? If not, you should say > "Each slice references an allocation context..." Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:81: // allocated for this context. The |cursor| of the slices indicates the current On 2015/12/04 19:13:20, petrcermak wrote: > If the previous comment applies, go to page 200 ... just joking ... in that > case, s/slices/slice/ Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:82: // position in the tree when breaking down. The branches taken are [begin, On 2015/12/04 19:13:20, petrcermak wrote: > "branches taken" is difficult to understand. What about "the patch taken in the > tree so far is [begin, cursor)"? Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:106: return Part(property == &Part::backtrace ? backtrace.next() : backtrace, On 2015/12/04 19:13:20, petrcermak wrote: > Thought: Is it worth keeping copying backtrace/type_name over using smart > pointers here? I did not measure, but I think it is. Instead of unique ownership + copy we could change to shared ownership, which means refcounting in base. A |Slice| is only three pointers big, so refcounting would save two pointers for every owner at the expense of the size of the counter plus the |Slice| itself for every instance. The size of the backtrace is 8 frames on average, so this would save ~12 pointers on that path if it gets broken down entirely, at the expense of a heap allocation, an extra pointer dereference for every access, and the refcounting overhead (but it is not atomic so that should be fine). Performance aside, I would opt for simplicity unless this turns out to be a bottleneck. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:152: return buckets; On 2015/12/04 19:13:20, petrcermak wrote: > Are move semantics already allowed in Chrome? If they are, this might be a good > place to use them (if I'm not mistaken). Even before C++11, compilers would do Return Value Optimisation. The compiler isn’t really going to copy construct the vector at the call site, instead it silently adds an extra argument to the function that means “construct the return value here”. Now that rvalue references are a thing and |std::vector| has a move constructor, the vector at the call site will use the move constructor, because the return value of a function is an rvalue. You get this automatically when you compile with a C++11 standard library, regardless of whether Chrome allows it or not :) There is no need to put a |std::move| anywhere, if that is what you mean. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:192: return buckets; On 2015/12/04 19:13:20, petrcermak wrote: > ditto See above. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:220: // If the type name is not set use ID -1, otherwise deduplicate the type name. On 2015/12/04 19:13:20, petrcermak wrote: > supernit: I'd say "ID = -1" (multiple uses in this patch). Feel free to ignore. Ignored. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:31: // the processing, and pass it to |Write| to get a |TracedValue| that can be On 2015/12/04 19:13:20, petrcermak wrote: > nit: I'd say "... and pass its return value to |Write| ...". Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:62: // in the "entries" array. Use |Write| to convert to a traced value. On 2015/12/04 19:13:20, petrcermak wrote: > It would probably be worth saying here that this also decides which values are > "important" and which are not. Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:78: // A map of allocation context to the number of bytes allocated for that On 2015/12/04 19:13:20, petrcermak wrote: > nit: s/map of/map from/ Done. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right): https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:53: // Note: Using Gtest |ASSERT_TRUE| instead of |DCHECK| causes a compile error. On 2015/12/04 19:13:20, petrcermak wrote: > Not sure this is a good idea because DCHECK won't run on a Release > build (unless you compile it with a special flag). You are right. Looking into it, |ASSERT_TRUE| can only be used in void functions [1], but |EXPECT_TRUE| is fine. (If the expectation is false the test will crash on null pointer dereference.) 1: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#my-co... https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:85: TEST(HeapDumpWriterTest, EmptyBacktraceIndexIsEmptyString) { On 2015/12/04 19:13:20, petrcermak wrote: > I don't think the test name matches what the test does. You also test a > non-empty backtrace. Fixed. https://codereview.chromium.org/1494293005/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:104: TEST(HeapDumpWriterTest, CumulativeTypeEntryOmitsTypeId) { On 2015/12/04 19:13:20, petrcermak wrote: > ditto Fixed.
After offline discussion, I removed the helper structs (Slice and Part) in favour of |std::pair|. At that point, treating type name and backtrace uniformly made little sense any more, so I changed it to be more ad-hoc. Please take another look.
https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:40: enum class Property { kBacktrace, kTypeName }; nit: s/Property/BreakdownMode/ or GroupingMode (as you used the word Group in the method) and add "By" to the constatns, i.e. kByBacktrace, kByTypeName https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:45: using Part = std::pair<const AllocationContext*, size_t>; As discussed offline, the name is confusing. Don't give it a name :) https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:50: std::vector<Part> parts; this becomes: // This is allocations grouped by size. std::vector<std::pair<AllocationContext*,size_t>> allocations; https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:61: bool type_name_broken_down; is_broken_down_by_type https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:64: bool operator<(const Bucket& lhs, const Bucket& rhs) { add a comment explaining what this is this used for. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:72: base::hash_map<const char*, Bucket> grouped; s/grouped/breakdown/ https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: for (const Part& part : bucket.parts) { yeah this is way more readable if you mention AllocationContext here. you are really iterating across allocations https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: for (const Part& part : bucket.parts) { maybe it's cleaner if this for is outside the ifs? https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:98: if (!bucket.type_name_broken_down) { should this be a DCHECK + an if somewhere else (where you do the recursion) instead? https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:151: // Discard the long tail of buckets that contribute less than a percent. Ok please add a comment which explains graphically the situation of the buckets at the end, something like begin() |it| end() | | | V V V [1][5][19] | [80][90][100] ^ ^ | +-Sorted (in reverse) sequence of top contributors. | +-heap of buckets that contribute for less than X https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:171: bool HeapDumpWriter::InsertEntry(const Bucket& bucket) { This needs a better name. At this point I'm confused: this is inserting the buckets you broke down into what? (I know the answer but I had to read the code to figure out). For a moment I confused with InsertAllocation. I think the real problem is that you are artificially splitting InsertEntry and BrakeDown but they are not really decoupled (in fact BrakeDown is the only caller of InsertEntry). I'd rework this as follows: One single method that does: void AppendResultAndBreakDownFurtherIfNecessary(bucket, Set<Entry>* results) { ... do all the insertion logic here... if (did already insert) return; for (subbucket : BreadDownBy(...backtrace) AppendResultAndBreakDownFurtherIfNecessary(subbucket); if (bucket.is_break_down_by_type) return; // No need to break down futher by type. for (subbucket : BreadDownBy(...type) AppendResultAndBreakDownFurtherIfNecessary(subbucket); } Set<Entry> should become a local variable of Dump(), not a field of the class. FWIW This will also give you the guarantee that Dump() works under all cicumstances, not only the first time you invoke it. I don't really care about that, it's just mentally cleaner: the set is something that you need only to do the dump, is not part of the state of the dumper. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:199: std::vector<Bucket> by_type_name = BreakDownBy(bucket, Property::kTypeName); this should be conditioned by if (bucket.is_broken_down...) Doesn't make lot of sense to breakdown by type if you know that the current one is already broken down, right? https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:64: const std::set<Entry>& Dump(); I understand this is for testability, but I'd like to see here only the method that make sense for this class (GetHeapDump that returns a TracedValue). You can make this a private method used internally and friend the test. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:68: static scoped_refptr<TracedValue> Write(const std::set<Entry>& dump); and obviously drop (privatize) this
I addressed most comments, but threading |entries| through all functions as argument instead of making it a member caused a bit of a waterfall of changes so before I continue any further please take another look. There is something to be said for (almost) pure functions but in my opinion this is messier than the previous version. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:40: enum class Property { kBacktrace, kTypeName }; On 2015/12/08 18:05:30, Primiano Tucci wrote: > nit: s/Property/BreakdownMode/ or GroupingMode (as you used the word Group in > the method) > > and add "By" to the constatns, i.e. kByBacktrace, kByTypeName I renamed |GroupBy| to |BreakDownBy|, because if this is to be named |BreakDownMode|, it does not make sense to use it for grouping; grouping and breaking down are opposites. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:45: using Part = std::pair<const AllocationContext*, size_t>; On 2015/12/08 18:05:30, Primiano Tucci wrote: > As discussed offline, the name is confusing. Don't give it a name :) Done. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:50: std::vector<Part> parts; On 2015/12/08 18:05:30, Primiano Tucci wrote: > this becomes: > > // This is allocations grouped by size. > std::vector<std::pair<AllocationContext*,size_t>> allocations; Done. (const AllocationContext* though.) https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:61: bool type_name_broken_down; On 2015/12/08 18:05:30, Primiano Tucci wrote: > is_broken_down_by_type Done. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:64: bool operator<(const Bucket& lhs, const Bucket& rhs) { On 2015/12/08 18:05:30, Primiano Tucci wrote: > add a comment explaining what this is this used for. Done. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:72: base::hash_map<const char*, Bucket> grouped; On 2015/12/08 18:05:30, Primiano Tucci wrote: > s/grouped/breakdown/ Done. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: for (const Part& part : bucket.parts) { On 2015/12/08 18:05:30, Primiano Tucci wrote: > yeah this is way more readable if you mention AllocationContext here. you are > really iterating across allocations Iterating (AllocationContext, size_t) pairs, you mean? "allocations" is not entirely correct, these are allocations grouped by context, not individual allocations. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: for (const Part& part : bucket.parts) { On 2015/12/08 18:05:30, Primiano Tucci wrote: > maybe it's cleaner if this for is outside the ifs? Probably but then we can’t early out on type name if it is already broken down. (Or break immediately at the first iteration, but that fees weird to me.) On the other hand perhaps this should not be called in the first place if further breakdown is not possible. See also comment in |BreakDown|. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:98: if (!bucket.type_name_broken_down) { On 2015/12/08 18:05:30, Primiano Tucci wrote: > should this be a DCHECK + an if somewhere else (where you do the recursion) > instead? It could but that would entangle the two functions. See also comment in |BreakDown|. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:151: // Discard the long tail of buckets that contribute less than a percent. On 2015/12/08 18:05:30, Primiano Tucci wrote: > Ok please add a comment which explains graphically the situation of the buckets > at the end, something like > > begin() |it| end() > | | | > V V V > [1][5][19] | [80][90][100] > ^ ^ > | +-Sorted (in reverse) sequence of top contributors. > | > +-heap of buckets that contribute for less than X Done. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:171: bool HeapDumpWriter::InsertEntry(const Bucket& bucket) { On 2015/12/08 18:05:30, Primiano Tucci wrote: > This needs a better name. At this point I'm confused: this is inserting the > buckets you broke down into what? (I know the answer but I had to read the code > to figure out). For a moment I confused with InsertAllocation. > > I think the real problem is that you are artificially splitting InsertEntry and > BrakeDown but they are not really decoupled (in fact BrakeDown is the only > caller of InsertEntry). > > I'd rework this as follows: > > One single method that does: > > void AppendResultAndBreakDownFurtherIfNecessary(bucket, Set<Entry>* results) { > ... do all the insertion logic here... > if (did already insert) > return; > > for (subbucket : BreadDownBy(...backtrace) > AppendResultAndBreakDownFurtherIfNecessary(subbucket); > > if (bucket.is_break_down_by_type) > return; // No need to break down futher by type. > > for (subbucket : BreadDownBy(...type) > AppendResultAndBreakDownFurtherIfNecessary(subbucket); > } > > > Set<Entry> should become a local variable of Dump(), not a field of the class. > FWIW This will also give you the guarantee that Dump() works under all > cicumstances, not only the first time you invoke it. > I don't really care about that, it's just mentally cleaner: the set is something > that you need only to do the dump, is not part of the state of the dumper. Of course you never really _need_ classes or methods at all, you can always make a method a function by taking |this| as an argument explicitly, and then you can get rid of the class by passing all of its members as arguments. You do realise that by passing |Entry| as an argument, there is no reason to have any members in |HeapDumpWriter| any more, right? (The deduplicators also need to be passed as argument if you want to pass |entries| as argument, because creating an |Entry| requires them.) At that point, |bytes_by_context_| and |InsertAllocation| are just a boilerplate wrapper around |base::hash_map|, so the entire class can be removed. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:199: std::vector<Bucket> by_type_name = BreakDownBy(bucket, Property::kTypeName); On 2015/12/08 18:05:30, Primiano Tucci wrote: > this should be conditioned by if (bucket.is_broken_down...) > Doesn't make lot of sense to breakdown by type if you know that the current one > is already broken down, right? Yes, you are right. I wanted to avoid further asymmetry / future maintenance cost by not doing the conditional here. We can move it here and avoid the conditional in |GroupBy| but I feel like that spreads logic all over the place. For type name we know when further breakdown is possible, but for type name we don’t know that in general. Just to confirm, you are proposing to put the burden of checking whether breakdown is possible on the caller if you pass |kByTypeName|, but on the callee if you pass |kByBacktrace|? https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:64: const std::set<Entry>& Dump(); On 2015/12/08 18:05:30, Primiano Tucci wrote: > I understand this is for testability, but I'd like to see here only the method > that make sense for this class (GetHeapDump that returns a TracedValue). You can > make this a private method used internally and friend the test. Obsolete now. I put it in a namespace instead. https://codereview.chromium.org/1494293005/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:68: static scoped_refptr<TracedValue> Write(const std::set<Entry>& dump); On 2015/12/08 18:05:30, Primiano Tucci wrote: > and obviously drop (privatize) this Obsolete now.
LGTM with a few more comments. I agree with you that the original version was nicer :-( Petr https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:23: // heap and deciding what to dump. Input to this process is a list of nit: I think there should be "The" in front of "Input" https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:27: // sizes of which the properties share a prefix. (Type name is considered a hnit: It sounds like you're talking about "properties of sizes" here, but I think you want to say "properties of contexts". Not sure how to avoid splitting this into two sentences. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:68: // |is_broken_down_by_type_name| set depending the property to group by. nit: "depending ON" https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:73: if (breakDownBy == BreakDownMode::kByBacktrace) { Wouldn't a switch make more sense here? https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: const AllocationContext* context = context_and_size.first; You always use |context| to get the backtrace so you might as well store context_and_size.first->backtrace straightaway. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:173: // [begin, cursor) range is equal for all contexts in the bucket. This comment is outdated - it doesn't capture the fact that types IDs are not a list anymore.
I gave it another try, mostly similar to patchset 4 but with the |HeapDumper| class hidden as an implementation detail. This has the advantage of exposing |WriteHeapDump| as an (almost) pure function while still enabling testing of the individual steps. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:23: // heap and deciding what to dump. Input to this process is a list of On 2015/12/09 15:02:31, petrcermak wrote: > nit: I think there should be "The" in front of "Input" Done. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:27: // sizes of which the properties share a prefix. (Type name is considered a On 2015/12/09 15:02:31, petrcermak wrote: > hnit: It sounds like you're talking about "properties of sizes" here, but I > think you want to say "properties of contexts". Not sure how to avoid splitting > this into two sentences. Done. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:68: // |is_broken_down_by_type_name| set depending the property to group by. On 2015/12/09 15:02:32, petrcermak wrote: > nit: "depending ON" Done. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:73: if (breakDownBy == BreakDownMode::kByBacktrace) { On 2015/12/09 15:02:32, petrcermak wrote: > Wouldn't a switch make more sense here? I don’t have a strong opinion. I find a switch to be a bit awkward because you need the |break| and there is a block here anyway. An advantage of switch is that some compilers warn about non-exhaustive switch statements. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:75: const AllocationContext* context = context_and_size.first; On 2015/12/09 15:02:31, petrcermak wrote: > You always use |context| to get the backtrace so you might as well store > context_and_size.first->backtrace straightaway. Done. https://codereview.chromium.org/1494293005/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:173: // [begin, cursor) range is equal for all contexts in the bucket. On 2015/12/09 15:02:32, petrcermak wrote: > This comment is outdated - it doesn't capture the fact that types IDs > are not a list anymore. Fixed. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:222: BreakDown(subbucket); I prefer to keep the call to |BreakDown| here instead of moving it into |AddEntryForBucket|, because then this function is recursive directly. If we move it into |AddEntryForBucket|, then there is the |BreakDown| -> |AddEntryForBucket| -> |BreakDown| -> |AddEntryForBucket| -> ... recursion, and it might be harder to see what is going on. I did rename |InsertEntry| to |AddEntryForBucket|.
LGTM with some (Really) minor comments. Many thanks for the patience, I know this took long but I think now is way more readable than the original version. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context.h:85: struct BASE_EXPORT hash<base::trace_event::Backtrace> { just doublechecking: does this need to be exported because you need it in the unittests (which in component builds are a different link unit?) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:9: #include <set> remove, you have this in the .h https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:10: #include <tuple> I think this is not needed anymore https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:17: #include "base/macros.h" remove, you have this in the .h https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:44: struct BASE_EXPORT Bucket { don't think this needs BASE_EXPORT https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:162: buckets.erase(buckets.begin(), it); I wonder whether resize() here is more efficient or erase(begin()) is smart enough. (just curious, non need to change the code) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:222: BreakDown(subbucket); On 2015/12/09 16:16:01, Ruud van Asseldonk wrote: > I prefer to keep the call to |BreakDown| here instead of moving it into > |AddEntryForBucket|, because then this function is recursive directly. If we > move it into |AddEntryForBucket|, then there is the |BreakDown| -> > |AddEntryForBucket| -> |BreakDown| -> |AddEntryForBucket| -> ... recursion, and > it might be harder to see what is going on. > > I did rename |InsertEntry| to |AddEntryForBucket|. Acknowledged. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:229: const std::set<Entry>& HeapDumpWriter::Dump( I wonder if we can get a better name here. I think this is really something like "Reduce" or "SummarizeAndCluster" or something like that. Dump is a bit generic. I think the key thing you want to convey in the name is that here you are doing some smart bucketing + threshold . https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:26: // entries is kept reasonable because long tails are not inpluded. typo on inpluded https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:27: BASE_EXPORT scoped_refptr<TracedValue> WriteHeapDump( thinking on naming: is this really a "Write" function? THis is not really writing anywhere (other than retuning a tracedvalue) I wonder whether this should be called "ConvertHeapDump" "ExportHeapDump" "CompactAndSerializeHeapDump" or something like that? https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:52: BASE_EXPORT bool operator<(Entry lhs, Entry rhs); Since you have Entry declared here, can you not make this a member function of it (by adding a bool operator<(const Entry& foo) method ?) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:54: // Writes an "entries" array to a traced value. See https://goo.gl/KY7zVE for probably s/Writes/Serializes/ https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:54: // Writes an "entries" array to a traced value. See https://goo.gl/KY7zVE for I think it's the 3rd time you mention the link to the doc. One should be enough :)
https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context.h:85: struct BASE_EXPORT hash<base::trace_event::Backtrace> { On 2015/12/09 17:03:59, Primiano Tucci wrote: > just doublechecking: does this need to be exported because you need it in the > unittests (which in component builds are a different link unit?) content/child also uses it now. The |AllocationContext| hash, that is. |Backtrace| doesn’t strictly need to be exported, but it was bad style because it is in the headers, so putting this in a hash map outside of base would compile perfectly fine and then fail to link. I should have exported this in the first place, but I forgot. (And you missed it when reviewing :) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:9: #include <set> On 2015/12/09 17:04:00, Primiano Tucci wrote: > remove, you have this in the .h Done. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:10: #include <tuple> On 2015/12/09 17:03:59, Primiano Tucci wrote: > I think this is not needed anymore It is required for |std::tie| which is required for |operator<|. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:17: #include "base/macros.h" On 2015/12/09 17:03:59, Primiano Tucci wrote: > remove, you have this in the .h Done. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:44: struct BASE_EXPORT Bucket { On 2015/12/09 17:03:59, Primiano Tucci wrote: > don't think this needs BASE_EXPORT You are right. Fixed. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.cc:162: buckets.erase(buckets.begin(), it); On 2015/12/09 17:04:00, Primiano Tucci wrote: > I wonder whether resize() here is more efficient or erase(begin()) is smart > enough. (just curious, non need to change the code) Resize discards elements at the end, I want to discard elements at the beginning. I hope that |erase| is smart enough to move every element at most once but I did not check this. I considered using a |deque| instead for efficient removal from the front, but |deque| does not have |reserve|. Either way we shouln’t optimise prematurely. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:26: // entries is kept reasonable because long tails are not inpluded. On 2015/12/09 17:04:00, Primiano Tucci wrote: > typo on inpluded Fixed. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:27: BASE_EXPORT scoped_refptr<TracedValue> WriteHeapDump( On 2015/12/09 17:04:00, Primiano Tucci wrote: > thinking on naming: is this really a "Write" function? THis is not really > writing anywhere (other than retuning a tracedvalue) > I wonder whether this should be called "ConvertHeapDump" "ExportHeapDump" > "CompactAndSerializeHeapDump" or something like that? I agree, |ExportHeapDump| is better. (The file is called writer though.) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:52: BASE_EXPORT bool operator<(Entry lhs, Entry rhs); On 2015/12/09 17:04:00, Primiano Tucci wrote: > Since you have Entry declared here, can you not make this a member function of > it (by adding a bool operator<(const Entry& foo) method ?) Yes that would be possible. I didn’t because I think it is cleaner to have a struct without members. https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:54: // Writes an "entries" array to a traced value. See https://goo.gl/KY7zVE for On 2015/12/09 17:04:00, Primiano Tucci wrote: > I think it's the 3rd time you mention the link to the doc. One should be enough > :) Removed. This is what happens when you move things around five times :) https://codereview.chromium.org/1494293005/diff/120001/base/trace_event/heap_... base/trace_event/heap_profiler_heap_dump_writer.h:54: // Writes an "entries" array to a traced value. See https://goo.gl/KY7zVE for On 2015/12/09 17:04:00, Primiano Tucci wrote: > probably s/Writes/Serializes/ Done. Also renamed method to |Serialize| because like you said, it doesn’t really write anything.
ruuda@google.com changed reviewers: + jam@chromium.org
+jam, can you please take a look at this small change in content/child?
lgtm
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org, primiano@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1494293005/#ps160001 (title: "Rename _all_ occurrences")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494293005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494293005/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Enable heap dumps with both type info and backtraces This is a major change to |HeapDumpWriter| which enables it to dump the format described in https://goo.gl/KY7zVE. This will allow the heap profiler to break down the heap by type or backtrace, but also combinations of those. This changes the interface of |HeapDumpWriter| slightly from a two-step process (insert allocations, dump json) to a three-step process (insert allocations, compute what to dump, convert to json). The main reason for doing this is testability. Aggregation and json conversion are now completely orthogonal. This means that the aggregation algorithm can be tested without having to parse the json first, and the json conversion is simpler to test too. This is part of the heap profiler in chrome://tracing. BUG=524631 ========== to ========== [Tracing] Enable heap dumps with both type info and backtraces This is a major change to |HeapDumpWriter| which enables it to dump the format described in https://goo.gl/KY7zVE. This will allow the heap profiler to break down the heap by type or backtrace, but also combinations of those. This changes the interface of |HeapDumpWriter| slightly from a two-step process (insert allocations, dump json) to a three-step process (insert allocations, compute what to dump, convert to json). The main reason for doing this is testability. Aggregation and json conversion are now completely orthogonal. This means that the aggregation algorithm can be tested without having to parse the json first, and the json conversion is simpler to test too. This is part of the heap profiler in chrome://tracing. BUG=524631 Committed: https://crrev.com/3314e2e367138427f3aaccd3da030413c045ab56 Cr-Commit-Position: refs/heads/master@{#364444} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3314e2e367138427f3aaccd3da030413c045ab56 Cr-Commit-Position: refs/heads/master@{#364444} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
