Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by ssid Modified:
4 years, 8 months ago Reviewers:
Primiano Tucci (use gerrit), Dmitry Skiba, Nico CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, Maria, DmitrySkiba Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Ignore tracing allocations in heap profiler
This CL removes the memory usage of tracing from the heap profiler.
Adds support for an ignore event which clears the stack frame and
type for the allocations happening in that scope and reset them to
"overhead". We do not make it null because it will show under "<other>"
or "[unknown]".
Adds the ignore events to major functions that allocate memory for
tracing. The total is usually 500Kib more than the total in "tracing"
dump.
BUG=604689
Committed: https://crrev.com/04d1750c6721a424ae45e7e28f71dba69f1789db
Cr-Commit-Position: refs/heads/master@{#389159}
Patch Set 1 : #Patch Set 2 : nits. #
Total comments: 21
Patch Set 3 : Define in new file. #Patch Set 4 : fixes. #
Total comments: 20
Patch Set 5 : make heap_profiler.h and rebase #
Total comments: 13
Patch Set 6 : fix year. #Patch Set 7 : remove cc file and inline. #
Created: 4 years, 8 months ago
Messages
Total messages: 40 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [tracing] Remove tracing overhead from heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds an ignore malloc event which clears the stack frame and type for the allocations happening in that scope and reset to overhead. BUG=604689 ========== to ========== [tracing] Remove tracing overhead from heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore malloc event which clears the stack frame and type for the allocations happening in that scope and reset them to overhead. Adds the ignore events to major functions that allocate memory for tracing. BUG=604689 ==========
Patchset #1 (id:20001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
+primiano I have added events as you suggested. ptal thanks.
Description was changed from ========== [tracing] Remove tracing overhead from heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore malloc event which clears the stack frame and type for the allocations happening in that scope and reset them to overhead. Adds the ignore events to major functions that allocate memory for tracing. BUG=604689 ========== to ========== [tracing] Remove tracing overhead from heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore malloc event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ==========
Ok conceptually this makes perfect sense. I think we need some small improvements. THe major bummer is that we should not cause the creation of the TLS ContextTracker if heap profiling is not enabled but that is easy to fix. see comments below. Thanks a lot for doing this so quickly. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:125: ctx = AllocationContext::Empty(); I think either you or dskiba need to rebase on top of each other :) https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:55: void start_ignore_scope() { ignore_scope_count_++; } add a comment plz. One for both is ok. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:56: void end_ignore_scope() { s/start/begin/ s/start/end/ https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:92: uint32_t ignore_scope_count_; s/count/depth/ https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.h (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:23: #define SCOPED_HEAP_PROFILER_IGNORE_MALLOC_EVENT \ HEAP_PROFILER_SCOPED_IGNORE should be enough as a name More importantly I think you want here an if UNLIKELY(capture_enabled()) to prevent initializing the TLS tracker when heap profiling is not used. Which means that you should not increase the counter in the ctor, but I think this needs to be #define HEAP_PROFILER_SCOPED_IGNORE \ ScopedHeapProfilerIgnore INTERNAL_TRACE_EVENT_UID(heap_profiler_ignore) if (UNLIKELY(capture_enabled()) heap_profiler_ignore.begin_ignore(); In essence, you need an empty ctor in ScopedHeapProfilerIgnore , and the dtor should call the end_ignore() only if the begin was called. This is really the same identical thing of ScopedTracer https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:24: base::trace_event::MallocDumpProvider::ScopedHeapProfilerIgnoreMalloc event Ok I know I suggested malloc_dump_provider in the first place, but by looking at your cl i just realized that this is really general and not malloc specific (and should be such). In other words, this scoped ignore will cause an ignore to ALL the dump providers not just malloc. It's ok, I think is just a matter of moving this macro and the scoped bject to a better, more generic place. Either let's put this (and the class gbelow) in heap_profiler_allocation_context_tracker.h or let's create heap_profiler_scoped_ignore.h (probably the latter is better, so can be used without pulling in weird deps) https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:34: class ScopedHeapProfilerIgnoreMalloc { I think I'd drop Malloc from the name, for the reason explained above (this works also for any other client). ScopedHeapProfilerIgnore I think is ok https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... File base/trace_event/trace_buffer.cc (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... base/trace_event/trace_buffer.cc:35: SCOPED_HEAP_PROFILER_IGNORE_MALLOC_EVENT; I think you want to move this only to line 52 which does the new? Nothing else here should do a malloc
dskiba@google.com changed reviewers: + dskiba@google.com
https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:92: uint32_t ignore_scope_count_; Also, size_t is better here.
Thanks. Made changes, ptal. > THe major bummer is that we should not cause the creation of the TLS > ContextTracker if heap profiling is not enabled but that is easy to fix. This wasn't happening in my previous patch too with check inside the constructor. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:125: ctx = AllocationContext::Empty(); On 2016/04/20 16:34:57, Primiano Tucci wrote: > I think either you or dskiba need to rebase on top of each other :) Yes was waiting for it to land. Done. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:55: void start_ignore_scope() { ignore_scope_count_++; } On 2016/04/20 16:34:57, Primiano Tucci wrote: > add a comment plz. One for both is ok. Sorry uploaded older patch here. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:56: void end_ignore_scope() { On 2016/04/20 16:34:57, Primiano Tucci wrote: > s/start/begin/ > s/start/end/ I think you meant begin and end here. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:92: uint32_t ignore_scope_count_; On 2016/04/20 16:59:15, Dmitry Skiba wrote: > Also, size_t is better here. Done. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:92: uint32_t ignore_scope_count_; On 2016/04/20 16:34:57, Primiano Tucci wrote: > s/count/depth/ Done. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.h (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:23: #define SCOPED_HEAP_PROFILER_IGNORE_MALLOC_EVENT \ On 2016/04/20 16:34:57, Primiano Tucci wrote: > HEAP_PROFILER_SCOPED_IGNORE should be enough as a name > > More importantly I think you want here an if UNLIKELY(capture_enabled()) to > prevent initializing the TLS tracker when heap profiling is not used. > Which means that you should not increase the counter in the ctor, but I think > this needs to be > > #define HEAP_PROFILER_SCOPED_IGNORE \ > ScopedHeapProfilerIgnore INTERNAL_TRACE_EVENT_UID(heap_profiler_ignore) > if (UNLIKELY(capture_enabled()) > heap_profiler_ignore.begin_ignore(); > > In essence, you need an empty ctor in ScopedHeapProfilerIgnore , and the dtor > should call the end_ignore() only if the begin was called. > This is really the same identical thing of ScopedTracer > Yes I have already prevented the initialization of tracker by checking enabled inside the constuctor. Now moved it out so that constructor need not be called. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:24: base::trace_event::MallocDumpProvider::ScopedHeapProfilerIgnoreMalloc event On 2016/04/20 16:34:57, Primiano Tucci wrote: > Ok I know I suggested malloc_dump_provider in the first place, but by looking at > your cl i just realized that this is really general and not malloc specific (and > should be such). > In other words, this scoped ignore will cause an ignore to ALL the dump > providers not just malloc. > It's ok, I think is just a matter of moving this macro and the scoped bject to a > better, more generic place. > Either let's put this (and the class gbelow) in > heap_profiler_allocation_context_tracker.h or let's create > heap_profiler_scoped_ignore.h (probably the latter is better, so can be used > without pulling in weird deps) Yes, I was expecting this comment. I still let this be inside malloc since I was planning to move invalidation of context also inside this file. But, I think it is better to have it generic for all providers. I had to add a new file because it causes weird deps. I cannot use INTERNAL_TRACE_EVENT_UID in context_tracker.h because trace_event.h already includes context_tracker.h. The other option was to add this as TRACE_EVENT_API_HEAP_PROFILER_SCOPED_IGNORE. I think this is better. Maybe we should have a trace_event_heap_profiler_common file which defines new trace events which are specific to heap profiler. This way I can move the TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT, HEAP_PROFILER_SCOPED_IGNORE and in future I would also need TRACE_EVENT_API_ADD_NATIVE_STACK_EVENT or similar. I think this file heap_profiler_scoped_ignore will be moved to that. I will discuss this change later. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.h:34: class ScopedHeapProfilerIgnoreMalloc { On 2016/04/20 16:34:57, Primiano Tucci wrote: > I think I'd drop Malloc from the name, for the reason explained above (this > works also for any other client). > ScopedHeapProfilerIgnore I think is ok Done. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... File base/trace_event/trace_buffer.cc (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... base/trace_event/trace_buffer.cc:35: SCOPED_HEAP_PROFILER_IGNORE_MALLOC_EVENT; On 2016/04/20 16:34:57, Primiano Tucci wrote: > I think you want to move this only to line 52 which does the new? Nothing else > here should do a malloc There is a chunks_.resize call too. When we can capture this, why not. https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... base/trace_event/trace_buffer.cc:162: std::unique_ptr<TraceBufferChunk> GetChunk(size_t* index) override { I am sure that a ghost had removed this line from my CL!
LGTM but please wait for dskiba to do another pass and give his LGTM. I think your code is not rebased yet, some files don't match dskiba@ changes on ToT. Btw, I think at this point you can name the header just heap_profiler.h and we put all the common code there. Thanks a lot https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... File base/trace_event/trace_buffer.cc (right): https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... base/trace_event/trace_buffer.cc:35: SCOPED_HEAP_PROFILER_IGNORE_MALLOC_EVENT; On 2016/04/21 01:07:46, ssid wrote: > On 2016/04/20 16:34:57, Primiano Tucci wrote: > > I think you want to move this only to line 52 which does the new? Nothing else > > here should do a malloc > > There is a chunks_.resize call too. When we can capture this, why not. ahh right https://codereview.chromium.org/1900223003/diff/60001/base/trace_event/trace_... base/trace_event/trace_buffer.cc:162: std::unique_ptr<TraceBufferChunk> GetChunk(size_t* index) override { On 2016/04/21 01:07:46, ssid wrote: > I am sure that a ghost had removed this line from my CL! uh? right you need one here https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:124: if (ignore_scope_depth_) { This file doesn't seem rebased. The ToT code is different after dskiba's CL. Anyways, here you are missing a fundamental: ctx.backtrace.frame_count = 1 :) https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:56: // ignored in the heap profiler. A dummy context is returned for these I'd say: A dummy context that short circuits to "tracing_overhead" is returned... https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:60: DCHECK(ignore_scope_depth_); thinking more I think you should remove the dcheck and just do if(ignore_scope_depth) ignore_scope_depth--; I think there is a rare case where you can hit this case legitimately, Imagine the following sequence: (tracing is not enabled yet) Thread 1: begin_scoped_ignore() do something Thread 2: enable tracing, now capture_enabled == true Thread 1: end_scoped_ignore() When T1 gets to end_scoped_ignore the depth was 0 because tracing was not enabled before. So you might hit the dcheck (and wrap back the depth). I'd just add the if and leave happy without the dcheck https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:291: ASSERT_FALSE(ctx1.backtrace.frames[1]); this is not guaranteed to be nullptr anymore. This should check that frame_count == 1 https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_scoped_ignore.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.cc:12: HeapProfilerScopedIgnore::~HeapProfilerScopedIgnore() { I'd just add a comment saying: The corresponding begin_ignore_scope is in the definition of HEAP_PROFILER_SCOPED_IGNORE in the header. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_scoped_ignore.h (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:5: #ifndef BASE_TRACE_EVENT_HEAP_PROFILER_SCOPED_IGNORE_H if you want feel free to call this file already heap_profiler.h and then in another cl move the rest. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:9: #include "base/trace_event/trace_event.h" you are including this only for INTERNAL_TRACE_UID right? To be honest, since trace_event is a complex header, why not doing just (below) ScopedIgnore heap_profiler_ignore#__LINE__ ? Might be just simpler https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:15: base::trace_event::AllocationContextTracker::capture_enabled())) { \ remove the curly braces and make this if (UNLIKELY(....)) ...GetInstanceForCurrentThread()->begin_ignore_scope() without a trailing smicolon so that when you write HEAP_PROFILER_SCOPED_IGNORE; it makes sense and you don't get warning about null statements. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:23: // A scoped ignore event used to tell AllocationContextTracker to ignore all the s/AllocationCOntextTracker/the memory-infra heap profiler/ https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/trace... base/trace_event/trace_log.cc:912: HEAP_PROFILER_SCOPED_IGNORE; out of curiosity, why this is needed here? This should happen only after the end of the trace, when you will no longer emit any memory dump, right?
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
thanks, made changes. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:124: if (ignore_scope_depth_) { On 2016/04/21 19:47:31, Primiano Tucci wrote: > This file doesn't seem rebased. > The ToT code is different after dskiba's CL. > Anyways, here you are missing a fundamental: > > ctx.backtrace.frame_count = 1 :) Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:56: // ignored in the heap profiler. A dummy context is returned for these On 2016/04/21 19:47:31, Primiano Tucci wrote: > I'd say: > A dummy context that short circuits to "tracing_overhead" is returned... Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:60: DCHECK(ignore_scope_depth_); On 2016/04/21 19:47:31, Primiano Tucci wrote: > thinking more I think you should remove the dcheck and just do > > if(ignore_scope_depth) > ignore_scope_depth--; > > I think there is a rare case where you can hit this case legitimately, Imagine > the following sequence: > > > (tracing is not enabled yet) > Thread 1: > begin_scoped_ignore() > do something > Thread 2: > enable tracing, now capture_enabled == true > Thread 1: > end_scoped_ignore() > > When T1 gets to end_scoped_ignore the depth was 0 because tracing was not > enabled before. > So you might hit the dcheck (and wrap back the depth). > I'd just add the if and leave happy without the dcheck Makes sense. done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:291: ASSERT_FALSE(ctx1.backtrace.frames[1]); On 2016/04/21 19:47:32, Primiano Tucci wrote: > this is not guaranteed to be nullptr anymore. > This should check that frame_count == 1 Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_scoped_ignore.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.cc:12: HeapProfilerScopedIgnore::~HeapProfilerScopedIgnore() { On 2016/04/21 19:47:32, Primiano Tucci wrote: > I'd just add a comment saying: > The corresponding begin_ignore_scope is in the definition of > HEAP_PROFILER_SCOPED_IGNORE in the header. Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_scoped_ignore.h (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:5: #ifndef BASE_TRACE_EVENT_HEAP_PROFILER_SCOPED_IGNORE_H On 2016/04/21 19:47:32, Primiano Tucci wrote: > if you want feel free to call this file already heap_profiler.h > and then in another cl move the rest. Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:9: #include "base/trace_event/trace_event.h" On 2016/04/21 19:47:32, Primiano Tucci wrote: > you are including this only for INTERNAL_TRACE_UID right? > To be honest, since trace_event is a complex header, why not doing just (below) > > ScopedIgnore heap_profiler_ignore#__LINE__ ? > Might be just simpler This does not seem to work. I had to add macros similar to TRACE_EVENT_UID. heap_profiler_ignore#__LINE__ gives: heap_profiler_ignore#40 With one level of indirection and ## it gives: heap_profiler_ignore__LINE__ if I write a # in second level it gives: heap_profiler_ignore"__LINE__" With second level only it gives: heap_profiler_ignore40. I couldn't find a better way to do it. I am copying the TRACE_EVENT_UID implementation. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:15: base::trace_event::AllocationContextTracker::capture_enabled())) { \ On 2016/04/21 19:47:32, Primiano Tucci wrote: > remove the curly braces and make this > if (UNLIKELY(....)) > ...GetInstanceForCurrentThread()->begin_ignore_scope() > without a trailing smicolon > so that when you write > > > HEAP_PROFILER_SCOPED_IGNORE; > it makes sense and you don't get warning about null statements. Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_scoped_ignore.h:23: // A scoped ignore event used to tell AllocationContextTracker to ignore all the On 2016/04/21 19:47:32, Primiano Tucci wrote: > s/AllocationCOntextTracker/the memory-infra heap profiler/ Done. https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1900223003/diff/100001/base/trace_event/trace... base/trace_event/trace_log.cc:912: HEAP_PROFILER_SCOPED_IGNORE; On 2016/04/21 19:47:32, Primiano Tucci wrote: > out of curiosity, why this is needed here? This should happen only after the end > of the trace, when you will no longer emit any memory dump, right? Hm, I added this because if there are multiple tracing sessions then this usage might show up. Or it might not because the memory will get freed. I just added to be safe and not include it any time.
dskiba@ could you please take a look to see if the rebase is correct? Thanks!
ssid@chromium.org changed reviewers: + thakis@chromium.org
+Nico: Please look at base/BUILD.gn. Added 2 files. Thanks!
https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:29: INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore); \ Why do you need this at all, if it's only used once? Let it be INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore, __LINE__), and then you'll only need one level of indirection, i.e. EVENT_UID(a, b) -> EVENT_UID2(a, b) -> a##b However, I'm not insisting on this. You can push as is. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:33: ->begin_ignore_scope() Wait, why this is not in HeapProfilerScopedIngnore' constructor?
https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/22 05:37:45, Dmitry Skiba wrote: > 2016 Done. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:29: INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore); \ On 2016/04/22 05:37:45, Dmitry Skiba wrote: > Why do you need this at all, if it's only used once? Let it be > INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore, __LINE__), and then you'll only > need one level of indirection, i.e. EVENT_UID(a, b) -> EVENT_UID2(a, b) -> a##b > > However, I'm not insisting on this. You can push as is. Yes, but at least the user of this macro doesn't have to include a __LINE__ every time, the other parts look cleaner. More over this is same as impl of INTERNAL_TRACE_EVENT_UID https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:33: ->begin_ignore_scope() On 2016/04/22 05:37:45, Dmitry Skiba wrote: > Wait, why this is not in HeapProfilerScopedIngnore' constructor? Primiano had suggested that keeping this out reduces a call to constructor itself. this gets inlined.
https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:29: INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore); \ On 2016/04/22 05:48:27, ssid wrote: > On 2016/04/22 05:37:45, Dmitry Skiba wrote: > > Why do you need this at all, if it's only used once? Let it be > > INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore, __LINE__), and then you'll > only > > need one level of indirection, i.e. EVENT_UID(a, b) -> EVENT_UID2(a, b) -> > a##b > > > > However, I'm not insisting on this. You can push as is. > > Yes, but at least the user of this macro doesn't have to include a __LINE__ > every time, the other parts look cleaner. More over this is same as impl of > INTERNAL_TRACE_EVENT_UID Actually, wait. Why it's called INTERNAL_HEAP_PROFILER_EVENT_UID? It's not related to any event stuff, right? INTERNAL_TRACE_EVENT_UID makes sense because it's used in trace_event implementation. This should be just INTERNAL_HEAP_PROFILER_UID I think. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:33: ->begin_ignore_scope() On 2016/04/22 05:48:27, ssid wrote: > On 2016/04/22 05:37:45, Dmitry Skiba wrote: > > Wait, why this is not in HeapProfilerScopedIngnore' constructor? > > Primiano had suggested that keeping this out reduces a call to constructor > itself. this gets inlined. How about inline constructor then? Actually, how about inlining both of them?
https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:15: // Implementation detail: heap profiler macros create temporary variables to this is really overkill, you are adding 7 lines, and you would get here the same benefit of just doing, below in the macro #define HEAP_PROFILER_SCOPED_IGNORE \ trace_event_internal::HeapProfilerScopedIgnore scoped_ignore#__LINE__ which is way easier to read. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:33: ->begin_ignore_scope() On 2016/04/22 06:01:20, Dmitry Skiba wrote: > On 2016/04/22 05:48:27, ssid wrote: > > On 2016/04/22 05:37:45, Dmitry Skiba wrote: > > > Wait, why this is not in HeapProfilerScopedIngnore' constructor? > > > > Primiano had suggested that keeping this out reduces a call to constructor > > itself. this gets inlined. > > How about inline constructor then? Actually, how about inlining both of them? inlining both SGTM and might be even cleaner (also you'll avoid the need of the cc file).
ah very small note: I think the title of this CL should be changed to: [tracing] ignore tracing allocations in heap profiler I would NOT mention overhead here, because it conflicts in my mind with the overhead estimation crbug.com/602992 which is completely orthogonal. Plz let's land these CLs or you guys will spend your time rebasing on top of each other :)
https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:15: // Implementation detail: heap profiler macros create temporary variables to On 2016/04/22 09:24:50, Primiano Tucci wrote: > this is really overkill, you are adding 7 lines, and you would get here the same > benefit of just doing, below in the macro > > #define HEAP_PROFILER_SCOPED_IGNORE \ > trace_event_internal::HeapProfilerScopedIgnore scoped_ignore#__LINE__ > > which is way easier to read. As I said in earlier mail, this doesn't work. It prints scoped_ignore#40. There's no easy way I could find to get it without 3 macros.
Description was changed from ========== [tracing] Remove tracing overhead from heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore malloc event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ========== to ========== [tracing] Ignore tracing allocations in heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ==========
Made changes except the UID macro. Thanks. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler.h (right): https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:29: INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore); \ On 2016/04/22 06:01:20, Dmitry Skiba wrote: > On 2016/04/22 05:48:27, ssid wrote: > > On 2016/04/22 05:37:45, Dmitry Skiba wrote: > > > Why do you need this at all, if it's only used once? Let it be > > > INTERNAL_HEAP_PROFILER_EVENT_UID(scoped_ignore, __LINE__), and then you'll > > only > > > need one level of indirection, i.e. EVENT_UID(a, b) -> EVENT_UID2(a, b) -> > > a##b > > > > > > However, I'm not insisting on this. You can push as is. > > > > Yes, but at least the user of this macro doesn't have to include a __LINE__ > > every time, the other parts look cleaner. More over this is same as impl of > > INTERNAL_TRACE_EVENT_UID > > Actually, wait. Why it's called INTERNAL_HEAP_PROFILER_EVENT_UID? It's not > related to any event stuff, right? INTERNAL_TRACE_EVENT_UID makes sense because > it's used in trace_event implementation. This should be just > INTERNAL_HEAP_PROFILER_UID I think. Done. https://codereview.chromium.org/1900223003/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler.h:33: ->begin_ignore_scope() On 2016/04/22 09:24:50, Primiano Tucci wrote: > On 2016/04/22 06:01:20, Dmitry Skiba wrote: > > On 2016/04/22 05:48:27, ssid wrote: > > > On 2016/04/22 05:37:45, Dmitry Skiba wrote: > > > > Wait, why this is not in HeapProfilerScopedIngnore' constructor? > > > > > > Primiano had suggested that keeping this out reduces a call to constructor > > > itself. this gets inlined. > > > > How about inline constructor then? Actually, how about inlining both of them? > > inlining both SGTM and might be even cleaner (also you'll avoid the need of the > cc file). Done.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900223003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900223003/220001
build.gn lgtm Maybe it makes sense to have a BUILD.gn in your folder that defines a source_set for this and have base/BUILD.gn pull that in?
On 2016/04/22 16:28:10, Nico wrote: > build.gn lgtm > > Maybe it makes sense to have a BUILD.gn in your folder that defines a source_set > for this and have base/BUILD.gn pull that in? I think it used to be like that until https://codereview.chromium.org/1540953003 .
PS7 LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900223003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900223003/220001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Ignore tracing allocations in heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ========== to ========== [tracing] Ignore tracing allocations in heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Ignore tracing allocations in heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 ========== to ========== [tracing] Ignore tracing allocations in heap profiler This CL removes the memory usage of tracing from the heap profiler. Adds support for an ignore event which clears the stack frame and type for the allocations happening in that scope and reset them to "overhead". We do not make it null because it will show under "<other>" or "[unknown]". Adds the ignore events to major functions that allocate memory for tracing. The total is usually 500Kib more than the total in "tracing" dump. BUG=604689 Committed: https://crrev.com/04d1750c6721a424ae45e7e28f71dba69f1789db Cr-Commit-Position: refs/heads/master@{#389159} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/04d1750c6721a424ae45e7e28f71dba69f1789db Cr-Commit-Position: refs/heads/master@{#389159}
Message was sent while issue was closed.
Thanks for the pointer! Left a question at https://codereview.chromium.org/1540953003/ On Fri, Apr 22, 2016 at 1:04 PM, <primiano@chromium.org> wrote: > On 2016/04/22 16:28:10, Nico wrote: > > build.gn lgtm > > > > Maybe it makes sense to have a BUILD.gn in your folder that defines a > source_set > > for this and have base/BUILD.gn pull that in? > > I think it used to be like that until > https://codereview.chromium.org/1540953003 > . > > > https://codereview.chromium.org/1900223003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
