Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760253005:
memory-infra: Fill the memory dump callback result (1/2) (Closed)
Descriptionmemory-infra: Fill the memory dump callback result (1/2)
Fill in the part of the memory dump result struct
that applies to Chrome's veiw of itself when finalizing
a memory dump. This requires a tweak to memory_allocator_dump
to keep track of the 'size' field.
BUG=703184
Review-Url: https://codereview.chromium.org/2760253005
Cr-Commit-Position: refs/heads/master@{#459506}
Committed: https://chromium.googlesource.com/chromium/src/+/adbce8ba658534687a82e731ef138678f116f3a2
Patch Set 1 #Patch Set 2 : tweak a few comments #
Total comments: 18
Patch Set 3 : update for comments #
Total comments: 8
Patch Set 4 : update for comments #Patch Set 5 : fix typo #
Total comments: 8
Patch Set 6 : fix nits #Patch Set 7 : rebase for struct change #
Total comments: 3
Patch Set 8 : fill struct only in detailed mode #
Total comments: 6
Patch Set 9 : fix nits #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 29 (10 generated)
hjd@chromium.org changed reviewers: + fmeawad@chromium.org, primiano@chromium.org
ptal, thanks!
Thanks makes great sense, just minor comments. A note about the CL title. I usually put some context label to allow sheriffs (or random people who stumble across this cl) to understand which part of the code this is touching. I'd do something like: memory-infra: Fill the memory dump callback result (1/2) https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump.cc:67: if (strcmp(kNameSize, name) == 0 && strcmp(kUnitsBytes, units) == 0) { I like your paranoy, but purely for sake of perf, I'd just keep the first strcmp. If somebody pushes a "size" which is a uint but NOT bytes they deserve the worst. :P https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump.h:68: uint64_t GetSize(); can you plz: - make this private (or protected, if it helps from a documentation viewpoint) - make MDM a friend of this class - Add a TODO(hjd) explaining that this is transitional, untile we get to the state where the full PMD can be sent over IPC/MOJO (better if you file a bug explaining this and link it to the todo) https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_unittest.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_unittest.cc:182: dump->AddScalar(MemoryAllocatorDump::kNameSize, "foo", 3); repeating the same scalar name multiple times is undefined behavior (we just don't bother CHECKing that, I'd remove this line from the test. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:727: if (kv.first.compare(0, prefix.length(), prefix) == 0) { as we discussed offline let's use StartsWith in string_util.h because, oh boy, C++ is hard https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:731: // Convert bytes to kilobytes. this comment should be unnecessary given the name :) https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:754: // Fill results struct. ditto about todo + bug https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:756: for (const auto& kv : pmd_async_state->process_dumps) { since you have the same loop below, could you make this part of that? (I guess there is some other reason for keeping a separate loop I did overlook) https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:762: SumSizesInKbForPrefix("malloc/", process_memory_dump); hmm I know that we discussed this offline but now I wonder if this is going to count also the suballocations of malloc obtained via AddSuballocation (sorry didn't think to malloc before). On one side the suballocations should be only part of DETAILED dumps, so as long as we limit this to BACKGROUND for the moment everything should work (but maybe you could add a DCHECK for "/__" in the SumSizesInKbForPrefix that checks that we don't accidentally use this with things that have suballocations). https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:300: static uint32_t SumSizesInKbForPrefix(const std::string&, I'd call this "GetDumpsSumKb" (to make clear that this is about summing the "dump", and use an explicit |prefix| in the string var name (so you don't end up repeating "prefix" in the .cc file) Also I'd add a similar comment with todo saying this is transitional with link to the bug
Description was changed from ========== Fill in half of the memory dump result struct Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 ========== to ========== memory-infra: Fill in half of the memory dump result struct Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 ==========
Description was changed from ========== memory-infra: Fill in half of the memory dump result struct Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 ========== to ========== memory-infra: Fill the memory dump callback result (1/2) Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 ==========
https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump.cc:67: if (strcmp(kNameSize, name) == 0 && strcmp(kUnitsBytes, units) == 0) { On 2017/03/22 17:19:52, Primiano Tucci wrote: > I like your paranoy, but purely for sake of perf, I'd just keep the first > strcmp. > If somebody pushes a "size" which is a uint but NOT bytes they deserve the > worst. :P sounds good to me :) https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump.h:68: uint64_t GetSize(); On 2017/03/22 17:19:52, Primiano Tucci wrote: > can you plz: > - make this private (or protected, if it helps from a documentation viewpoint) > - make MDM a friend of this class > - Add a TODO(hjd) explaining that this is transitional, untile we get to the > state where the full PMD can be sent over IPC/MOJO (better if you file a bug > explaining this and link it to the todo) Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_unittest.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_unittest.cc:182: dump->AddScalar(MemoryAllocatorDump::kNameSize, "foo", 3); On 2017/03/22 17:19:52, Primiano Tucci wrote: > repeating the same scalar name multiple times is undefined behavior (we just > don't bother CHECKing that, I'd remove this line from the test. Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:727: if (kv.first.compare(0, prefix.length(), prefix) == 0) { On 2017/03/22 17:19:52, Primiano Tucci wrote: > as we discussed offline let's use StartsWith in string_util.h because, oh boy, > C++ is hard Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:731: // Convert bytes to kilobytes. On 2017/03/22 17:19:52, Primiano Tucci wrote: > this comment should be unnecessary given the name :) Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:754: // Fill results struct. On 2017/03/22 17:19:52, Primiano Tucci wrote: > ditto about todo + bug Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:756: for (const auto& kv : pmd_async_state->process_dumps) { On 2017/03/22 17:19:52, Primiano Tucci wrote: > since you have the same loop below, could you make this part of that? (I guess > there is some other reason for keeping a separate loop I did overlook) not really, just seemed easier while I was debugging, I have combined them. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:762: SumSizesInKbForPrefix("malloc/", process_memory_dump); On 2017/03/22 17:19:52, Primiano Tucci wrote: > hmm I know that we discussed this offline but now I wonder if this is going to > count also the suballocations of malloc obtained via AddSuballocation (sorry > didn't think to malloc before). > On one side the suballocations should be only part of DETAILED dumps, so as long > as we limit this to BACKGROUND for the moment everything should work (but maybe > you could add a DCHECK for "/__" in the SumSizesInKbForPrefix that checks that > we don't accidentally use this with things that have suballocations). Done. https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2760253005/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:300: static uint32_t SumSizesInKbForPrefix(const std::string&, On 2017/03/22 17:19:52, Primiano Tucci wrote: > I'd call this "GetDumpsSumKb" (to make clear that this is about summing the > "dump", and use an explicit |prefix| in the string var name (so you don't end up > repeating "prefix" in the .cc file) > > Also I'd add a similar comment with todo saying this is transitional with link > to the bug Done.
ssid@chromium.org changed reviewers: + ssid@chromium.org
https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:732: DCHECK(name.find("/__") == name.npos); So, this is supposed to work only for BACKGROUND mode? If so, maybe update the description to say fill in the details only for background mode? https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:786: GetDumpsSumKb("malloc/", process_memory_dump); This seems very fragile. the double counting depends on just the difference between counting "malloc" and "malloc/". https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:796: GetDumpsSumKb("blink_gc/", process_memory_dump); So, in this case we would be counting the "allocated_objects" only and not counting the total. Whereas in other cases like malloc we count total. Basically this depends on if we create an explicit dump for the "unspecified" part. I think a better solution here is to have pattern matching. Use base::MatchPattern or similar. Use malloc/* for the case of malloc. Use blink_gc for blink gc's total. Or if we are only interested in allocated_objects size then we should just use malloc/allcoated_objects and blink_gc/allcoated_objects and partition_alloc/allcoated_objects.
Sorry for the back and forth, ssid made some good points. In summary I think we should make the following change: use base::Pattern so we can do either full string matching, or pattern matching (e.g. "malloc" vs "malloc/*") Report the following: "malloc" (i.e. only the total) "blink_gc" (i.e. only the total) "partition_alloc/partitions/*" "v8/*" This should work consistently in any dump mode, regardless of suballocations. ssid WDYT? https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:786: GetDumpsSumKb("malloc/", process_memory_dump); On 2017/03/23 03:11:38, ssid wrote: > This seems very fragile. the double counting depends on just the difference > between counting "malloc" and "malloc/". In this case I'd just count "malloc" (without any *, i.e. any recursion), which is the outer total size. This would remove any ambiguity about suballocations https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:796: GetDumpsSumKb("blink_gc/", process_memory_dump); On 2017/03/23 03:11:38, ssid wrote: > So, in this case we would be counting the "allocated_objects" only and not > counting the total. Whereas in other cases like malloc we count total. Basically > this depends on if we create an explicit dump for the "unspecified" part. We don't for blink_gc. Since we have an explicit total dump, here I'd get only "blink_gc" (Again, with no recursion). and like in the preivcous case, this should be fine regardless of BACKGROUND / DETAILED. Instead for PA we need partition_alloc/partitions/* > I think a better solution here is to have pattern matching. Use > base::MatchPattern or similar. > > Use malloc/* for the case of malloc. > Use blink_gc for blink gc's total. > > Or if we are only interested in allocated_objects size then we should just use > malloc/allcoated_objects and blink_gc/allcoated_objects and > partition_alloc/allcoated_objects.
https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:732: DCHECK(name.find("/__") == name.npos); On 2017/03/23 03:11:38, ssid wrote: > So, this is supposed to work only for BACKGROUND mode? > If so, maybe update the description to say fill in the details only for > background mode? I removed this now that it should work in all modes, thanks! https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:786: GetDumpsSumKb("malloc/", process_memory_dump); On 2017/03/23 12:27:58, Primiano Tucci wrote: > On 2017/03/23 03:11:38, ssid wrote: > > This seems very fragile. the double counting depends on just the difference > > between counting "malloc" and "malloc/". > > In this case I'd just count "malloc" (without any *, i.e. any recursion), which > is the outer total size. > This would remove any ambiguity about suballocations Done. https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:796: GetDumpsSumKb("blink_gc/", process_memory_dump); On 2017/03/23 12:27:58, Primiano Tucci wrote: > On 2017/03/23 03:11:38, ssid wrote: > > So, in this case we would be counting the "allocated_objects" only and not > > counting the total. Whereas in other cases like malloc we count total. > Basically > > this depends on if we create an explicit dump for the "unspecified" part. > We don't for blink_gc. > Since we have an explicit total dump, here I'd get only "blink_gc" (Again, with > no recursion). and like in the preivcous case, this should be fine regardless of > BACKGROUND / DETAILED. > > Instead for PA we need partition_alloc/partitions/* > > > I think a better solution here is to have pattern matching. Use > > base::MatchPattern or similar. > > > > Use malloc/* for the case of malloc. > > Use blink_gc for blink gc's total. > > > > Or if we are only interested in allocated_objects size then we should just use > > malloc/allcoated_objects and blink_gc/allcoated_objects and > > partition_alloc/allcoated_objects. > Done.
On 2017/03/23 14:50:31, hjd wrote: > https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... > File base/trace_event/memory_dump_manager.cc (right): > > https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_manager.cc:732: DCHECK(name.find("/__") == > name.npos); > On 2017/03/23 03:11:38, ssid wrote: > > So, this is supposed to work only for BACKGROUND mode? > > If so, maybe update the description to say fill in the details only for > > background mode? > > I removed this now that it should work in all modes, thanks! > > https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_manager.cc:786: GetDumpsSumKb("malloc/", > process_memory_dump); > On 2017/03/23 12:27:58, Primiano Tucci wrote: > > On 2017/03/23 03:11:38, ssid wrote: > > > This seems very fragile. the double counting depends on just the difference > > > between counting "malloc" and "malloc/". > > > > In this case I'd just count "malloc" (without any *, i.e. any recursion), > which > > is the outer total size. > > This would remove any ambiguity about suballocations > > Done. > > https://codereview.chromium.org/2760253005/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_manager.cc:796: GetDumpsSumKb("blink_gc/", > process_memory_dump); > On 2017/03/23 12:27:58, Primiano Tucci wrote: > > On 2017/03/23 03:11:38, ssid wrote: > > > So, in this case we would be counting the "allocated_objects" only and not > > > counting the total. Whereas in other cases like malloc we count total. > > Basically > > > this depends on if we create an explicit dump for the "unspecified" part. > > We don't for blink_gc. > > Since we have an explicit total dump, here I'd get only "blink_gc" (Again, > with > > no recursion). and like in the preivcous case, this should be fine regardless > of > > BACKGROUND / DETAILED. > > > > Instead for PA we need partition_alloc/partitions/* > > > > > I think a better solution here is to have pattern matching. Use > > > base::MatchPattern or similar. > > > > > > Use malloc/* for the case of malloc. > > > Use blink_gc for blink gc's total. > > > > > > Or if we are only interested in allocated_objects size then we should just > use > > > malloc/allcoated_objects and blink_gc/allcoated_objects and > > > partition_alloc/allcoated_objects. > > > > Done. Thanks for your comments ssid! :) I've updated the CL to use base::MatchPattern
thanks, LGTM with minor nits. Let's wait for ssid reply though to check that the numbers make sense https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump.cc:67: if (strcmp(kNameSize, name) == 0) { nit: for consistency with the rest of the code in this file, can you drop the braces for single line if (IIRC our code style allows both but says to stay locally consistent) https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump.h:108: FRIEND_TEST_ALL_PREFIXES(MemoryAllocatorDumpTest, GetSize); nit: IIRC this should go up just below friend. https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:729: if (MatchPattern(name, pattern)) { ditto about braces https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:786: // partition_alloc reports sizes for both allocated_objects and nit: add a \n before this comment to split from the previous line, so it's clearer it refers to the one below
https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump.cc:67: if (strcmp(kNameSize, name) == 0) { On 2017/03/23 15:50:49, Primiano Tucci wrote: > nit: for consistency with the rest of the code in this file, can you drop the > braces for single line if (IIRC our code style allows both but says to stay > locally consistent) Done. https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump.h:108: FRIEND_TEST_ALL_PREFIXES(MemoryAllocatorDumpTest, GetSize); On 2017/03/23 15:50:49, Primiano Tucci wrote: > nit: IIRC this should go up just below friend. Done. https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:729: if (MatchPattern(name, pattern)) { On 2017/03/23 15:50:49, Primiano Tucci wrote: > ditto about braces Done. https://codereview.chromium.org/2760253005/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:786: // partition_alloc reports sizes for both allocated_objects and On 2017/03/23 15:50:49, Primiano Tucci wrote: > nit: add a \n before this comment to split from the previous line, so it's > clearer it refers to the one below Done.
Sorry my last comment was with only BACKGROUND mode in mind. Now that you removed that condition more issues come to my mind. https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:780: if (pid == kNullProcessId) { This should still not work for detailed mode. I think we should just do this for LIGHT and BACKGROUND mode and shoot for DETAILED mode later. Or we could just add total dumps in V8 and partition_alloc instead of trying to pattern match. Again this can be done later and just using != DETAILED here would suffice. Buffer partition in partition alloc records lot of child nodes in detailed mode. partition_alloc/buffer -> 1MB partition_alloc/buffer/bucket1 -> 200KB partition_alloc/buffer/bucket3 -> 300KB partition_alloc/buffer/bucket2 -> 500KB Now with our pattern matching we will be recording 2MB for PartitionAlloc whereas we should be recording only 1. This also deosn't work for V8 in heavy mode with object stats enabled. But, this is a rare case since it requires command line flag to double count. The following 2 dumps record the same memory. v8/heap_spaces/heap_X v8/heap_objects_at_last_gc/object1
https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:780: if (pid == kNullProcessId) { On 2017/03/23 18:32:34, ssid wrote: > This should still not work for detailed mode. I think we should just do this for > LIGHT and BACKGROUND mode and shoot for DETAILED mode later. > Or we could just add total dumps in V8 and partition_alloc instead of trying to > pattern match. Again this can be done later and just using != DETAILED here > would suffice. > > Buffer partition in partition alloc records lot of child nodes in detailed mode. > partition_alloc/buffer -> 1MB > partition_alloc/buffer/bucket1 -> 200KB > partition_alloc/buffer/bucket3 -> 300KB > partition_alloc/buffer/bucket2 -> 500KB > Aaaand you are right (unless we make it so that the * expands only to one level, but it might be tricky) Fine for me to CHECK that for the moment we use only with level < DETAILED > Now with our pattern matching we will be recording 2MB for PartitionAlloc > whereas we should be recording only 1. > > This also deosn't work for V8 in heavy mode with object stats enabled. But, this > is a rare case since it requires command line flag to double count. > The following 2 dumps record the same memory. > v8/heap_spaces/heap_X > v8/heap_objects_at_last_gc/object1
Updated, thanks! https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:780: if (pid == kNullProcessId) { On 2017/03/23 19:01:25, Primiano Tucci wrote: > On 2017/03/23 18:32:34, ssid wrote: > > This should still not work for detailed mode. I think we should just do this > for > > LIGHT and BACKGROUND mode and shoot for DETAILED mode later. > > Or we could just add total dumps in V8 and partition_alloc instead of trying > to > > pattern match. Again this can be done later and just using != DETAILED here > > would suffice. > > > > Buffer partition in partition alloc records lot of child nodes in detailed > mode. > > partition_alloc/buffer -> 1MB > > partition_alloc/buffer/bucket1 -> 200KB > > partition_alloc/buffer/bucket3 -> 300KB > > partition_alloc/buffer/bucket2 -> 500KB > > > > Aaaand you are right (unless we make it so that the * expands only to one level, > but it might be tricky) > Fine for me to CHECK that for the moment we use only with level < DETAILED > > > Now with our pattern matching we will be recording 2MB for PartitionAlloc > > whereas we should be recording only 1. > > > > This also deosn't work for V8 in heavy mode with object stats enabled. But, > this > > is a rare case since it requires command line flag to double count. > > The following 2 dumps record the same memory. > > v8/heap_spaces/heap_X > > v8/heap_objects_at_last_gc/object1 > Done.
lgtm with some final nits https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:103: uint64_t GetSize(); ups sorry just realized this, two small nits: 1. functions should go above fields, so this should go before absolute name. 2. +const 3. trivial accessors can be just inlined in small case, so this could be just be just uint64_6 get_size() const { return size_; } https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:782: if (pmd_async_state->req_args.level_of_detail == since you have already an if below I'd just merge this with the if, and do if (pid == kNukk && lod != DETAILED) { ... }
https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:724: uint32_t MemoryDumpManager::GetDumpsSumKb(const std::string& pattern, ah, also just realized that this could be just a function in the anonynmous namespace (and hence remove it from the header)
https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:103: uint64_t GetSize(); On 2017/03/24 11:39:21, Primiano Tucci wrote: > ups sorry just realized this, two small nits: > 1. functions should go above fields, so this should go before absolute name. > 2. +const > 3. trivial accessors can be just inlined in small case, so this could be just be > just > > uint64_6 get_size() const { return size_; } Thanks, done! https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:724: uint32_t MemoryDumpManager::GetDumpsSumKb(const std::string& pattern, On 2017/03/24 11:41:27, Primiano Tucci wrote: > ah, also just realized that this could be just a function in the anonynmous > namespace (and hence remove it from the header) as we realized offline it would make accessing GetSize awkward :( https://codereview.chromium.org/2760253005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:782: if (pmd_async_state->req_args.level_of_detail == On 2017/03/24 11:39:22, Primiano Tucci wrote: > since you have already an if below I'd just merge this with the if, and do > > if (pid == kNukk && lod != DETAILED) { > ... > } That if is about to a have another case in about an hour when I land the 2/2 patch :) At which point we'll either have to do this or nest the ifs and I think that this way is cleaner.
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2760253005/#ps160001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hjd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490374549867010,
"parent_rev": "4994f7d782fd181925cb44265af9c5b21f7ee402", "commit_rev":
"adbce8ba658534687a82e731ef138678f116f3a2"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Fill the memory dump callback result (1/2) Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 ========== to ========== memory-infra: Fill the memory dump callback result (1/2) Fill in the part of the memory dump result struct that applies to Chrome's veiw of itself when finalizing a memory dump. This requires a tweak to memory_allocator_dump to keep track of the 'size' field. BUG=703184 Review-Url: https://codereview.chromium.org/2760253005 Cr-Commit-Position: refs/heads/master@{#459506} Committed: https://chromium.googlesource.com/chromium/src/+/adbce8ba658534687a82e731ef13... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/adbce8ba658534687a82e731ef13... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
