Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1100173002:
Add a WinHeap dump provider to the memory profiler. (Closed)
|
Created:
5 years, 8 months ago by Sébastien Marchand Modified:
5 years, 8 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a WinHeap dump provider to the memory profiler.
BUG=466141
Patch Set 1 : #
Total comments: 39
Patch Set 2 : Address comments. #Patch Set 3 : Update the GN build file. #
Messages
Total messages: 19 (4 generated)
sebmarchand@chromium.org changed reviewers: + chrisha@chromium.org, primiano@chromium.org
Patchset #1 (id:1) has been deleted
PTAL.
Many thanks. Just some comments here and there but looks great. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.gypi (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.gypi:55: 'trace_event/winheap_dump_provider_win.cc', I think that you can just drop the final _win suffix. Unforuntately, as you might have found out, you cannot benefit of the auto exclusion rule (which excludes *win.cc on Non-win platforms) if you are in a gypi file. ...at which point i'd just make call just winheap_dump_provider. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.gypi:72: 'trace_event/winheap_dump_provider_unittest.cc', You probably want the same conditional logic here? (or just move the conditions block above in lines 46-58 to the end, so you can use the same OS==win condition to augment both trace_event_sources and trace_event_Test_sources https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider.h (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider.h:8: #include <istream> out of curiosity, what do you need istream for in the header here? https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider.h:37: // Retrieves the information about given heap. The |heap_info| should contain Should this friend the test class? https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_unittest.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:18: class TestWinHeapDumpProvider : public WinHeapDumpProvider { Uhm a bit curious about this: it seems to not add anything to WinHeapDumpProvider, why don't you just use WinHeapDumpProvider directly? https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:42: winheap_dump_provider->allocator_attributes_type_info()); I think you are not using any attribute, right? this is going to be just a noop https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:78: block_to_skip.erase(block_to_skip.find(alloc)); I think you can omit the block_to_skip.find here and just erase(alloc) and leverage the eraser(const value_type& ) overload of std::set. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:23: DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd); This would never happen, and even if it happens, AFAIK the pattern in chromium is to not DCHECK for null pointers: they will just cause a segfault the first time you try to use them, so DCHECK is not going to add any particular extra value. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:43: DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd); ditto https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:57: block_to_skip.insert(all_heaps.get()); Can you add just a comment to explain why you are skipping the all_heaps struct? I do trust that there is a good reason for doing that, but by reading the code, it reads like "let's skip all heaps" :) https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:65: all_heap_info.allocated_size += heap_info.allocated_size; If you want you can ReportHeapDump the heaps invidivually here (using the a unique string each time) and the UI will do the math to add up (it's not there yet but will be soon) https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:88: if (block_to_skip.find(heap_entry.lpData) != block_to_skip.end()) You can do if (block_to_skip.count(heap_entry.lpData) == 1) and probably save some cycle per iteration
https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_unittest.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:20: using WinHeapDumpProvider::GetHeapInformation; As suggested by Primiano, if you simply 'friend' the test fixture, then you don't need to create a TestWinHeapDumpProvider promoting this function to public... https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:67: // ignore it. ignores* it https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:35: Remove extra blank line. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:53: return false; A very minor thing, but this kind of racy 'get count, then get objects' code can be made slightly more robust by running in a loop a fixed number of iterations (a common pattern seen elsewhere in Chrome for these types of accesses). Although I think the benefit is very small here, so probably safe this way. Just document the fact that this query is inherently racy? https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:57: block_to_skip.insert(all_heaps.get()); On 2015/04/22 23:48:32, Primiano Tucci wrote: > Can you add just a comment to explain why you are skipping the all_heaps struct? > I do trust that there is a good reason for doing that, but by reading the code, > it reads like "let's skip all heaps" :) From my reading it looks like Seb is trying not to dump information about heap structures used temporarily by the heap dumping code. Is this something that you guys actually care about? If so, do you provide an alternative heap to use for all memory dumping related code? https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:59: // Retrieves some metrics about each heaps. each heap*. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:65: all_heap_info.allocated_size += heap_info.allocated_size; On 2015/04/22 23:48:32, Primiano Tucci wrote: > If you want you can ReportHeapDump the heaps invidivually here (using the a > unique string each time) and the UI will do the math to add up (it's not there > yet but will be soon) Yeah, it would be good to report them using names like we discussed in that group meeting. Something like winheap/heap-#{HEAPID}, and winheap/heap-#{HEAPID}-process-heap, or what have you (not sure if there's a preferred naming style, lowercase, CamelCase, snake_style, dashes or underscores, what have you).
thanks, PTAnL. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.gypi (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.gypi:55: 'trace_event/winheap_dump_provider_win.cc', On 2015/04/22 23:48:32, Primiano Tucci wrote: > I think that you can just drop the final _win suffix. > Unforuntately, as you might have found out, you cannot benefit of the auto > exclusion rule (which excludes *win.cc on Non-win platforms) if you are in a > gypi file. > ...at which point i'd just make call just winheap_dump_provider. It looks like I can just use the _win suffix and get rid of the condition (https://code.google.com/p/gyp/wiki/GypUserDocumentation#Add_a_platform-specif...) https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.gypi:72: 'trace_event/winheap_dump_provider_unittest.cc', On 2015/04/22 23:48:32, Primiano Tucci wrote: > You probably want the same conditional logic here? (or just move the conditions > block above in lines 46-58 to the end, so you can use the same OS==win condition > to augment both trace_event_sources and trace_event_Test_sources Ditto. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider.h (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider.h:8: #include <istream> On 2015/04/22 23:48:32, Primiano Tucci wrote: > out of curiosity, what do you need istream for in the header here? It's a leftover from one of my experiments :), removed. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider.h:37: // Retrieves the information about given heap. The |heap_info| should contain On 2015/04/22 23:48:32, Primiano Tucci wrote: > Should this friend the test class? Done. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_unittest.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:18: class TestWinHeapDumpProvider : public WinHeapDumpProvider { On 2015/04/22 23:48:32, Primiano Tucci wrote: > Uhm a bit curious about this: it seems to not add anything to > WinHeapDumpProvider, why don't you just use WinHeapDumpProvider directly? I just used this to expose some protected function of WinHeapDumpProvider, it's how we usually exposed the protected members in my codebase. I've switched to marking the WinHeapDumpProvider class as friend of this test fixture). https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:20: using WinHeapDumpProvider::GetHeapInformation; On 2015/04/23 09:23:27, chrisha wrote: > As suggested by Primiano, if you simply 'friend' the test fixture, then you > don't need to create a TestWinHeapDumpProvider promoting this function to > public... Yeah, I still need to put an accessor in the WinHeapDumpProviderTest fixture because the function declared with TEST_F(... , ...) don't seem to members of this class. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:42: winheap_dump_provider->allocator_attributes_type_info()); On 2015/04/22 23:48:32, Primiano Tucci wrote: > I think you are not using any attribute, right? this is going to be just a noop Yeah, I thought that it was required for the |set_allocated_objects_count|-like functions to work, but it doesn't seem to be the case. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:67: // ignore it. On 2015/04/23 09:23:27, chrisha wrote: > ignores* it Done. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_unittest.cc:78: block_to_skip.erase(block_to_skip.find(alloc)); On 2015/04/22 23:48:32, Primiano Tucci wrote: > I think you can omit the block_to_skip.find here and just erase(alloc) and > leverage the eraser(const value_type& ) overload of std::set. Good point... Fixed. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:23: DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd); On 2015/04/22 23:48:32, Primiano Tucci wrote: > This would never happen, and even if it happens, AFAIK the pattern in chromium > is to not DCHECK for null pointers: they will just cause a segfault the first > time you try to use them, so DCHECK is not going to add any particular extra > value. One more difference between my codebase (Syzygy) and Chrome ! Removed. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:35: On 2015/04/23 09:23:28, chrisha wrote: > Remove extra blank line. Done. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:43: DCHECK_NE(reinterpret_cast<ProcessMemoryDump*>(nullptr), pmd); On 2015/04/22 23:48:32, Primiano Tucci wrote: > ditto Done. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:53: return false; On 2015/04/23 09:23:28, chrisha wrote: > A very minor thing, but this kind of racy 'get count, then get objects' code can > be made slightly more robust by running in a loop a fixed number of iterations > (a common pattern seen elsewhere in Chrome for these types of accesses). > > Although I think the benefit is very small here, so probably safe this way. Just > document the fact that this query is inherently racy? Yeah, in Chrome all the heaps get created at startup, added a comment about that. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:57: block_to_skip.insert(all_heaps.get()); On 2015/04/22 23:48:32, Primiano Tucci wrote: > Can you add just a comment to explain why you are skipping the all_heaps struct? > I do trust that there is a good reason for doing that, but by reading the code, > it reads like "let's skip all heaps" :) Acknowledged. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:57: block_to_skip.insert(all_heaps.get()); On 2015/04/23 09:23:28, chrisha wrote: > On 2015/04/22 23:48:32, Primiano Tucci wrote: > > Can you add just a comment to explain why you are skipping the all_heaps > struct? > > I do trust that there is a good reason for doing that, but by reading the > code, > > it reads like "let's skip all heaps" :) > > From my reading it looks like Seb is trying not to dump information about heap > structures used temporarily by the heap dumping code. Is this something that you > guys actually care about? If so, do you provide an alternative heap to use for > all memory dumping related code? Yeah, my goal is to doesn't include the memory used by this provider in the calculation. This way we can get a more accurate number of how much memory Chrome actually use. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:59: // Retrieves some metrics about each heaps. On 2015/04/23 09:23:28, chrisha wrote: > each heap*. Done. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:65: all_heap_info.allocated_size += heap_info.allocated_size; On 2015/04/23 09:23:28, chrisha wrote: > On 2015/04/22 23:48:32, Primiano Tucci wrote: > > If you want you can ReportHeapDump the heaps invidivually here (using the a > > unique string each time) and the UI will do the math to add up (it's not there > > yet but will be soon) > > Yeah, it would be good to report them using names like we discussed in that > group meeting. Something like winheap/heap-#{HEAPID}, and > winheap/heap-#{HEAPID}-process-heap, or what have you (not sure if there's a > preferred naming style, lowercase, CamelCase, snake_style, dashes or > underscores, what have you). I'm planning to report all the heaps individually, but currently the UI doesn't support it really well... There's at least 4-5 heaps per process (most of them aren't used, except for the process heap) and they all get displayed into a distinct column, so with 5 process you end up with an array of 5 lines and ~15 columns, so you have to scroll horizontally a lot to find the information you're looking for... So I'd prefer to make it work first (i.e. report only the all_heaps data) and I'll try to get more detailed data in a further CL. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:65: all_heap_info.allocated_size += heap_info.allocated_size; On 2015/04/22 23:48:32, Primiano Tucci wrote: > If you want you can ReportHeapDump the heaps invidivually here (using the a > unique string each time) and the UI will do the math to add up (it's not there > yet but will be soon) If it's not there yet then I'd prefer to stick with this approach, it's easier to test as is. And I'll still need this code later (when I'll report the details about each heap). https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:88: if (block_to_skip.find(heap_entry.lpData) != block_to_skip.end()) On 2015/04/22 23:48:32, Primiano Tucci wrote: > You can do if (block_to_skip.count(heap_entry.lpData) == 1) and probably save > some cycle per iteration Done.
LGTM thanks (just one comment). Can you just use BUG=466141 in the commit message instead (which is about actual allocators)? Thanks. https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/1100173002/diff/20001/base/trace_event/winhea... base/trace_event/winheap_dump_provider_win.cc:65: all_heap_info.allocated_size += heap_info.allocated_size; On 2015/04/23 10:52:04, Sébastien Marchand wrote: > On 2015/04/23 09:23:28, chrisha wrote: > > On 2015/04/22 23:48:32, Primiano Tucci wrote: > > > If you want you can ReportHeapDump the heaps invidivually here (using the a > > > unique string each time) and the UI will do the math to add up (it's not > there > > > yet but will be soon) > > > > Yeah, it would be good to report them using names like we discussed in that > > group meeting. Something like winheap/heap-#{HEAPID}, and > > winheap/heap-#{HEAPID}-process-heap, or what have you (not sure if there's a > > preferred naming style, lowercase, CamelCase, snake_style, dashes or > > underscores, what have you). > > I'm planning to report all the heaps individually, but currently the UI doesn't > support it really well... There's at least 4-5 heaps per process (most of them > aren't used, except for the process heap) and they all get displayed into a > distinct column, so with 5 process you end up with an array of 5 lines and ~15 > columns, so you have to scroll horizontally a lot to find the information you're > looking for... So I'd prefer to make it work first (i.e. report only the > all_heaps data) and I'll try to get more detailed data in a further CL. Yeah, makes sense. Maybe add a todo or file a bug just to make sure we don't lose track of it
primiano@chromium.org changed reviewers: + nduca@chromium.org
+nduca to stamp
lgtm
lgtm!
Thanks, I've opened crbug.com/480385 to track this.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1100173002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/62215af64daf642c492fc2002cffc287b2c3bb09 Cr-Commit-Position: refs/heads/master@{#326501}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/1101853002/ by jonross@chromium.org. The reason for reverting is: This change has broke the CQ build: http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29/....
I forgot to update the GN files, PTAnL. Flip the CQ bit if this lgty.
Message was sent while issue was closed.
On 2015/04/23 14:03:45, Sébastien Marchand wrote: > I forgot to update the GN files, PTAnL. Flip the CQ bit if this lgty. This is re-landing in https://codereview.chromium.org/1087133005/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
