Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1675183006:
tracing: add dump provider for malloc heap profiler (Closed)
|
Created:
4 years, 10 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 8 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@shim_layer_linux Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing: add dump provider for malloc heap profiler
Now that the allocator shim is landing (the base API landed in
crre.com/1675143004, Linux support in crrev.com/1781573002) we
can use that to implement the malloc heap profiler on top of that.
This CL leverages the new shim API to collect information about
malloc/free and injecting that into the heap profiler, leveraging
the same infrastructure we are already using for PartitionAlloc
and BlinkGC (oilpan).
Next steps
----------
Currently this heap profiler reports also the memory used by the
tracing subsystem itself, which is not desirable. Will come up
with some scoped suppression semantic to bypass the reporting.
BUG=550886
TBR=haraken
Committed: https://crrev.com/14c8b52dac1285bbf23514d05e6109aa7edc427f
Cr-Commit-Position: refs/heads/master@{#382505}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : ifdef_guards #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : ifdef when not used #Patch Set 8 : production version #
Total comments: 34
Patch Set 9 : Petrcermak review #Patch Set 10 : add comment #
Total comments: 9
Patch Set 11 : ssid review #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 32 (13 generated)
Description was changed from ========== Working malloc heap profiler BUG= ========== to ========== Working malloc heap profiler Depends on the shim layer CL: crrev.com/1675143004 $ git cl patch 1675143004 $ git cl patch 1675183006 # this cl GYP build with use_experimental_allocator_shim=1 GN build with use_experimental_allocator_shim=true chrome --enable-heap-profiling --trace-startup=-*,disabled-by-default-memory-infra BUG= ==========
Description was changed from ========== Working malloc heap profiler Depends on the shim layer CL: crrev.com/1675143004 $ git cl patch 1675143004 $ git cl patch 1675183006 # this cl GYP build with use_experimental_allocator_shim=1 GN build with use_experimental_allocator_shim=true chrome --enable-heap-profiling --trace-startup=-*,disabled-by-default-memory-infra BUG= ========== to ========== [Work In Progress] malloc heap profiler Depends on the shim layer CL: crrev.com/1675143004 $ git cl patch 1675143004 $ git cl patch 1675183006 # this cl GYP build with use_experimental_allocator_shim=1 GN build with use_experimental_allocator_shim=true chrome --enable-heap-profiling --trace-startup=-*,disabled-by-default-memory-infra BUG= ==========
Description was changed from ========== [Work In Progress] malloc heap profiler Depends on the shim layer CL: crrev.com/1675143004 $ git cl patch 1675143004 $ git cl patch 1675183006 # this cl GYP build with use_experimental_allocator_shim=1 GN build with use_experimental_allocator_shim=true chrome --enable-heap-profiling --trace-startup=-*,disabled-by-default-memory-infra BUG= ========== to ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 ==========
primiano@chromium.org changed reviewers: + petrcermak@chromium.org, ssid@chromium.org
PTAL!
Looks good overall. Here's a couple of comments. Thanks, Petr https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:23: reinterpret_cast<AllocationContextTracker*>(-1); Is this really a safe thing to do? Wouldn't it be better to store an explicit enum? https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:34: // re-entrancy in the malloc heap profiler, that will lazy initialize the nit: s/that/which/ (you can't have "that" after a comma, which should stay here in this case) https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:34: // re-entrancy in the malloc heap profiler, that will lazy initialize the nit: s/lazy/lazily/ https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: // thread-local context trackes on the first malloc seen, causing a nested s/trackes/trackers/ https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: // thread-local context trackes on the first malloc seen, causing a nested supernit: wouldn't "first encountered malloc" read a little better? https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() What do you mean by "w.r.t LIGHT vs DETAILED"? Don't you mean "even in LIGHT dumps"? https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() supernit: I don't think there should be a comma before "to" here. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() supernit: I think you should s/malloc()/malloc/ or even s/malloc()/'malloc'/ because 'malloc' is then shown in the tracing UI. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() nit: s/w.r.t/w.r.t./ and s/vs/vs./ https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:200: overhead.DumpInto("tracing/heap_profiler_malloc", pmd); thought: Wouldn't "tracing/malloc_heap_profiler" be better (easier to find in the UI)? https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:218: // Inser/RemoveAllocation below will no-op if the register is torn down. s/Inser/Insert/ https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:225: if (tid_dumping_heap_ != kInvalidThreadId && Can it ever happen that PlatformThread::CurrentId() == kInvalidThreadID? If not, the first case is unnecessary. Also applies to line 250. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:242: AutoLock lock(allocation_register_lock_); Can't the compiler move this definition to the top of the method (i.e. shouldn't lines 242-246 be in a block)? Also applies to lines 253-256. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:253: AutoLock lock(allocation_register_lock_); Is it definitely the case that AllocationContextTracker will be initialized at this point? If yes, you should probably add a comment explaining why. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.h (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.h:58: // When in OnMemoryDump(), this contains the current thread thread ID. s/thread thread/thread/ or "thread's"?
Uberthanks, -pedantic as usual. See replies inline https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:23: reinterpret_cast<AllocationContextTracker*>(-1); On 2016/03/11 11:09:23, petrcermak wrote: > Is this really a safe thing to do? Wouldn't it be better to store an explicit > enum? It's extremely unlikely that a pointer would end up @ -1, as that is a non-aligned address :-) I can't really put another enum aside, because this is stored in a TLS-slot, and TLS slots are a scarce resource. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:34: // re-entrancy in the malloc heap profiler, that will lazy initialize the On 2016/03/11 11:09:23, petrcermak wrote: > nit: s/that/which/ (you can't have "that" after a comma, which should stay here > in this case) Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:34: // re-entrancy in the malloc heap profiler, that will lazy initialize the On 2016/03/11 11:09:23, petrcermak wrote: > nit: s/lazy/lazily/ Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: // thread-local context trackes on the first malloc seen, causing a nested On 2016/03/11 11:09:23, petrcermak wrote: > s/trackes/trackers/ actually was "tracker" , just typoed r/s https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: // thread-local context trackes on the first malloc seen, causing a nested On 2016/03/11 11:09:23, petrcermak wrote: > supernit: wouldn't "first encountered malloc" read a little better? Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() On 2016/03/11 11:09:23, petrcermak wrote: > nit: s/w.r.t/w.r.t./ and s/vs/vs./ Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() On 2016/03/11 11:09:24, petrcermak wrote: > supernit: I don't think there should be a comma before "to" here. Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() On 2016/03/11 11:09:24, petrcermak wrote: > supernit: I think you should s/malloc()/malloc/ or even s/malloc()/'malloc'/ > because 'malloc' is then shown in the tracing UI. Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:173: // reported (w.r.t LIGHT vs DETAILED), to avoid oscillations in the malloc() On 2016/03/11 11:09:24, petrcermak wrote: > What do you mean by "w.r.t LIGHT vs DETAILED"? Don't you mean "even in LIGHT > dumps"? Ok reworded this. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:200: overhead.DumpInto("tracing/heap_profiler_malloc", pmd); On 2016/03/11 11:09:24, petrcermak wrote: > thought: Wouldn't "tracing/malloc_heap_profiler" be better (easier to find in > the UI)? In the beginning it was like that, but I made it consistent with the partition_alloc and blink_gc, which today already use heap_profiler_XXX https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:218: // Inser/RemoveAllocation below will no-op if the register is torn down. On 2016/03/11 11:09:24, petrcermak wrote: > s/Inser/Insert/ Done. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:225: if (tid_dumping_heap_ != kInvalidThreadId && On 2016/03/11 11:09:24, petrcermak wrote: > Can it ever happen that PlatformThread::CurrentId() == kInvalidThreadID? If not, > the first case is unnecessary. Also applies to line 250. Heh I knew that you would have commented this. You are conceptually right, but there is a perf reason for doing this. CurrentId() is slow on some platforms, as it might end up invoking a syscall (if the libc implementation is dumb). In most of the cases, I expect to not be in the reentrant situation here, in which case the first check will short-circuit the if and prevent the CurrentID() call at all. Good spot btw :) https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:242: AutoLock lock(allocation_register_lock_); On 2016/03/11 11:09:24, petrcermak wrote: > Can't the compiler move this definition to the top of the method (i.e. shouldn't > lines 242-246 be in a block)? Also applies to lines 253-256. nope. The compiler is not allowed to reorder instructions unless it is sure that there is no side effect, which is not the case for Lock (because uses atomics, and more in general is not inlined and defined into another translation unit). The reason why sometimes you see Lock in a scope, is if you want the effect of the lock to end before the end of the function, e.g., { Lock; do_something_locked(); } do_something_unlocked(); https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:253: AutoLock lock(allocation_register_lock_); On 2016/03/11 11:09:24, petrcermak wrote: > Is it definitely the case that AllocationContextTracker will be initialized at > this point? If yes, you should probably add a comment explaining why. Good spot. It's definitely NOT the case (I mean, it can be non-initialized), for instance, if the very first thing you intercept after installing the hooks is a free (very likely can happen). But the deal here is that we don't need the tracker in the RemoveAllocation() function, so we don't care if that's initialized or not. The reason why I bother initializing that in the malloc() is because I need it few statements below when I do AllocationContextTracker::GetContextSnapshot() and at that point it must have been initialized or a reentrancy will happen. But yet again, this was another excellent spot, could have been a nasty bug, thanks for the -pedantic mode. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.h (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.h:58: // When in OnMemoryDump(), this contains the current thread thread ID. On 2016/03/11 11:09:24, petrcermak wrote: > s/thread thread/thread/ or "thread's"? Done.
LGTM with one more comment. Thanks, Petr https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:200: overhead.DumpInto("tracing/heap_profiler_malloc", pmd); On 2016/03/11 13:57:57, Primiano wrote: > On 2016/03/11 11:09:24, petrcermak wrote: > > thought: Wouldn't "tracing/malloc_heap_profiler" be better (easier to find in > > the UI)? > > In the beginning it was like that, but I made it consistent with the > partition_alloc and blink_gc, which today already use heap_profiler_XXX Acknowledged. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:225: if (tid_dumping_heap_ != kInvalidThreadId && On 2016/03/11 13:57:57, Primiano wrote: > On 2016/03/11 11:09:24, petrcermak wrote: > > Can it ever happen that PlatformThread::CurrentId() == kInvalidThreadID? If > not, > > the first case is unnecessary. Also applies to line 250. > > Heh I knew that you would have commented this. You are conceptually right, but > there is a perf reason for doing this. > CurrentId() is slow on some platforms, as it might end up invoking a syscall (if > the libc implementation is dumb). > In most of the cases, I expect to not be in the reentrant situation here, in > which case the first check will short-circuit the if and prevent the CurrentID() > call at all. > Good spot btw :) Acknowledged. https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:253: AutoLock lock(allocation_register_lock_); On 2016/03/11 13:57:57, Primiano wrote: > On 2016/03/11 11:09:24, petrcermak wrote: > > Is it definitely the case that AllocationContextTracker will be initialized at > > this point? If yes, you should probably add a comment explaining why. > > Good spot. It's definitely NOT the case (I mean, it can be non-initialized), for > instance, if the very first thing you intercept after installing the hooks is a > free (very likely can happen). > But the deal here is that we don't need the tracker in the RemoveAllocation() > function, so we don't care if that's initialized or not. > The reason why I bother initializing that in the malloc() is because I need it > few statements below when I do AllocationContextTracker::GetContextSnapshot() > and at that point it must have been initialized or a reentrancy will happen. > > But yet again, this was another excellent spot, could have been a nasty bug, > thanks for the -pedantic mode. Thanks for the detailed explanation. It might be worth adding a short comment explaining this in here.
https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/140001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:253: AutoLock lock(allocation_register_lock_); On 2016/03/11 14:26:31, petrcermak wrote: > On 2016/03/11 13:57:57, Primiano wrote: > > On 2016/03/11 11:09:24, petrcermak wrote: > > > Is it definitely the case that AllocationContextTracker will be initialized > at > > > this point? If yes, you should probably add a comment explaining why. > > > > Good spot. It's definitely NOT the case (I mean, it can be non-initialized), > for > > instance, if the very first thing you intercept after installing the hooks is > a > > free (very likely can happen). > > But the deal here is that we don't need the tracker in the RemoveAllocation() > > function, so we don't care if that's initialized or not. > > The reason why I bother initializing that in the malloc() is because I need it > > few statements below when I do AllocationContextTracker::GetContextSnapshot() > > and at that point it must have been initialized or a reentrancy will happen. > > > > But yet again, this was another excellent spot, could have been a nasty bug, > > thanks for the -pedantic mode. > > Thanks for the detailed explanation. It might be worth adding a short comment > explaining this in here. Done
lgtm A few comments to think about, i don't have a strong preference. https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:89: // in practice the pseudo stack never grows higher than ~20 frames. It could happen when someone tries to trace recursive functions maybe? But trying to handle this case might not be worth the effort. https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:177: // Enclosing all the temporariy data structures in a scope, so that the heap s/temporariy/temporary https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:242: AllocationContext context = AllocationContextTracker::GetContextSnapshot(); I think the code would be more readable if GetContextSnapshot is a member of AllocationContext and not static, that way AllocationContext can handle all the re-entrancy. there will be no need to check here. code could be: auto tracker = AllocationContextTracker::GetThreadLocalTracker() if (!tracker) return; context = tracker->GetContextSnapshot(); This could avoid the 2 new functions that you are exposing: GetStateForCurrentThread and InitializeForCurrentThread. I wonder why GetThreadLocalTracker is made protected.
dskiba@google.com changed reviewers: + dskiba@google.com
https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:217: // Insert/RemoveAllocation below will no-op if the register is torn down. Hmm, why did you remove RemoveAllocatorDispatch() call from here? OnHeapProfilingEnabled(true) called for the second time will really mess allocator hooks by inserting g_allocator_hooks again.
Thanks all for the comment. I reworked the AllocationContextTracker to have a GetInstanceForCurrentThread() method which makes everything less awkward. Thanks ssid@ for the suggestion :) https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:89: // in practice the pseudo stack never grows higher than ~20 frames. On 2016/03/11 19:51:10, ssid wrote: > It could happen when someone tries to trace recursive functions maybe? > But trying to handle this case might not be worth the effort. Never seen this happening in practice. If we do I can implement a counter which counts up and down, without storing the extra items, to deal with that. https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:177: // Enclosing all the temporariy data structures in a scope, so that the heap On 2016/03/11 19:51:10, ssid wrote: > s/temporariy/temporary Done. https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:217: // Insert/RemoveAllocation below will no-op if the register is torn down. On 2016/03/15 19:36:03, Dmitry Skiba wrote: > Hmm, why did you remove RemoveAllocatorDispatch() call from here? Because, after chatting with chrisha, we decided to remove the RemoveAllocatorDispatch API form the shim as it was hard to implement it in a sane and thread-safe way. > OnHeapProfilingEnabled(true) called for the second time will really mess > allocator hooks by inserting g_allocator_hooks again. OnHeapProfilingEnabled will never be called twice. The reason is that once you start heap profiling, you cannot stop and start again, as you will unrecoverably lose data (missing all the malloc/new that happen in the meantime). The only supported case here is: turning heap profiling on and off only once, and never again. I added a comment to explain it explicitly here. https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:242: AllocationContext context = AllocationContextTracker::GetContextSnapshot(); On 2016/03/11 19:51:10, ssid wrote: > I think the code would be more readable if GetContextSnapshot is a member of > AllocationContext and not static, that way AllocationContext can handle all the > re-entrancy. there will be no need to check here. > > code could be: > auto tracker = AllocationContextTracker::GetThreadLocalTracker() > if (!tracker) > return; > context = tracker->GetContextSnapshot(); > > This could avoid the 2 new functions that you are exposing: > GetStateForCurrentThread and InitializeForCurrentThread. > > I wonder why GetThreadLocalTracker is made protected. Ok I though more about this and just exposed a GetInstanceForCurrentThread that return nullptr on re-entrant calls. I think the code is more readable now. Thanks a lot for the suggestion
lgtm. thanks the only bit that looks awkward is that the GetInstance can return nullptr, but it is ok.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1675183006/#ps200001 (title: "ssid review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675183006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675183006/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 ========== to ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 TBR=haraken ==========
primiano@chromium.org changed reviewers: + haraken@chromium.org
+TBR haraken for two small changes (+GetInstance) to the dump providers in PartitionAllocMemoryDumpProvider and BlinkGCDumpProvider
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675183006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675183006/200001
WebKit LGTM
Message was sent while issue was closed.
Description was changed from ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 TBR=haraken ========== to ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 TBR=haraken ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 TBR=haraken ========== to ========== tracing: add dump provider for malloc heap profiler Now that the allocator shim is landing (the base API landed in crre.com/1675143004, Linux support in crrev.com/1781573002) we can use that to implement the malloc heap profiler on top of that. This CL leverages the new shim API to collect information about malloc/free and injecting that into the heap profiler, leveraging the same infrastructure we are already using for PartitionAlloc and BlinkGC (oilpan). Next steps ---------- Currently this heap profiler reports also the memory used by the tracing subsystem itself, which is not desirable. Will come up with some scoped suppression semantic to bypass the reporting. BUG=550886 TBR=haraken Committed: https://crrev.com/14c8b52dac1285bbf23514d05e6109aa7edc427f Cr-Commit-Position: refs/heads/master@{#382505} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/14c8b52dac1285bbf23514d05e6109aa7edc427f Cr-Commit-Position: refs/heads/master@{#382505}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1822013002/ by gab@chromium.org. The reason for reverting is: Breaks Google Chrome Mac bot. Looks like it fails to find base/allocator/features.h for the pnacl build. FWICT features.h is a generated header that goes in out/Debug/gen/base/allocator/features.h and it isn't found in the NaCL build for some reason. Weird that it was not caught when this landed but rather much later. It probably depends on which part of the incremental build are triggered..? Missing dependency somewhere perhaps? Full error log: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/b... Snippet: (...) FAILED: cd ../../base; export BUILT_FRAMEWORKS_DIR=/b/build/slave/google-chrome-rel-mac/build/src/out/Release; export BUILT_PRODUCTS_DIR=/b/build/slave/google-chrome-rel-mac/build/src/out/Release; export CONFIGURATION=Release; export PRODUCT_NAME=base_nacl; export SDKROOT=/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk; export SRCROOT=/b/build/slave/google-chrome-rel-mac/build/src/out/Release/../../base; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/b/build/slave/google-chrome-rel-mac/build/src/out/Release; export TEMP_DIR="${TMPDIR}"; export XCODE_VERSION_ACTUAL=0511;python ../native_client/build/build_nexe.py --root .. --product-dir ../out/Release/xyz --config-name Release -t ../native_client/toolchain/ --arch pnacl --build newlib_plib --name ../out/Release/gen/tc_pnacl_newlib/lib/libbase_nacl.a --objdir ../out/Release/obj/base/base_nacl.gen/pnacl_newlib-pnacl/base_nacl "--include-dirs=../out/Release/gen/tc_pnacl_newlib/include .. \"../out/Release/gen\" .." "--compile_flags=-O2 -g -Wall -fdiagnostics-show-option -Werror -fno-strict-aliasing -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function \"-std=gnu++11\" " --gomadir /b/build/goma "--defines=\"__STDC_LIMIT_MACROS=1\" \"__STDC_FORMAT_MACROS=1\" \"_GNU_SOURCE=1\" \"_POSIX_C_SOURCE=199506\" \"_XOPEN_SOURCE=600\" \"DYNAMIC_ANNOTATIONS_ENABLED=1\" \"DYNAMIC_ANNOTATIONS_PREFIX=NACL_\" \"NACL_BUILD_ARCH=x86\" V8_DEPRECATION_WARNINGS \"CLD_VERSION=2\" \"__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0\" GOOGLE_CHROME_BUILD \"CR_CLANG_REVISION=263324-1\" ENABLE_RLZ \"USE_LIBJPEG_TURBO=1\" \"ENABLE_WEBRTC=1\" \"ENABLE_MEDIA_ROUTER=1\" USE_PROPRIETARY_CODECS ENABLE_PEPPER_CDMS ENABLE_CONFIGURATION_POLICY ENABLE_NOTIFICATIONS \"ENABLE_TOPCHROME_MD=1\" \"ENABLE_TASK_MANAGER=1\" \"ENABLE_EXTENSIONS=1\" \"ENABLE_PDF=1\" \"ENABLE_PLUGIN_INSTALLATION=1\" \"ENABLE_PLUGINS=1\" \"ENABLE_SESSION_SERVICE=1\" \"ENABLE_THEMES=1\" \"ENABLE_AUTOFILL_DIALOG=1\" \"ENABLE_PROD_WALLET_SERVICE=1\" \"ENABLE_PRINTING=1\" \"ENABLE_BASIC_PRINTING=1\" \"ENABLE_PRINT_PREVIEW=1\" \"ENABLE_SPELLCHECK=1\" \"USE_BROWSER_SPELLCHECKER=1\" \"ENABLE_CAPTIVE_PORTAL_DETECTION=1\" \"ENABLE_APP_LIST=1\" \"ENABLE_SETTINGS_APP=1\" \"ENABLE_SUPERVISED_USERS=1\" \"ENABLE_SERVICE_DISCOVERY=1\" \"ENABLE_HANGOUT_SERVICES_EXTENSION=1\" V8_USE_EXTERNAL_STARTUP_DATA FULL_SAFE_BROWSING SAFE_BROWSING_CSD SAFE_BROWSING_DB_LOCAL \"USE_LIBPCI=1\" \"USE_OPENSSL=1\" \"USE_OPENSSL_CERTS=1\" __STDC_CONSTANT_MACROS __STDC_FORMAT_MACROS BASE_IMPLEMENTATION SYSTEM_NATIVE_UTF8" "--link_flags=-B../out/Release/gen/tc_pnacl_newlib/lib " "--source-list=../out/gypfiles/base/pnacl_newlib.base_nacl.source_list.gypcmd" trace_event/malloc_dump_provider.cc:11:10: fatal error: 'base/allocator/features.h' file not found #include "base/allocator/features.h" ^ 1 error generated. FAILED with 1: /b/build/goma/gomacc ../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-clang++ -c trace_event/malloc_dump_provider.cc -o ../out/Release/obj/base/base_nacl.gen/pnacl_newlib-pnacl/base_nacl/malloc_dump_provider_eeff7b85.o -MD -MF ../out/Release/obj/base/base_nacl.gen/pnacl_newlib-pnacl/base_nacl/malloc_dump_provider_eeff7b85.d -O2 -g -Wall -fdiagnostics-show-option -Werror -fno-strict-aliasing -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function -std=gnu++11 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1 -D_POSIX_C_SOURCE=199506 -D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DGOOGLE_CHROME_BUILD -DCR_CLANG_REVISION=263324-1 -DENABLE_RLZ -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_HANGOUT_SERVICES_EXTENSION=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DBASE_IMPLEMENTATION -DSYSTEM_NATIVE_UTF8 -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0 -DNACL_BUILD_ARCH=pnacl -I../out/Release/gen/tc_pnacl_newlib/include -I.. -I../out/Release/gen -I.. -DNDEBUG -std=gnu++0x -Wno-deprecated-register Compile options: ['-O2', '-g', '-Wall', '-fdiagnostics-show-option', '-Werror', '-fno-strict-aliasing', '-Wno-unused-function', '-Wno-char-subscripts', '-Wno-c++11-extensions', '-Wno-unnamed-type-template-args', '-Wno-extra-semi', '-Wno-unused-private-field', '-Wno-char-subscripts', '-Wno-unused-function', '-std=gnu++11', '-D__STDC_LIMIT_MACROS=1', '-D__STDC_FORMAT_MACROS=1', '-D_GNU_SOURCE=1', '-D_POSIX_C_SOURCE=199506', '-D_XOPEN_SOURCE=600', '-DDYNAMIC_ANNOTATIONS_ENABLED=1', '-DDYNAMIC_ANNOTATIONS_PREFIX=NACL_', '-DV8_DEPRECATION_WARNINGS', '-DCLD_VERSION=2', '-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0', '-DGOOGLE_CHROME_BUILD', '-DCR_CLANG_REVISION=263324-1', '-DENABLE_RLZ', '-DUSE_LIBJPEG_TURBO=1', '-DENABLE_WEBRTC=1', '-DENABLE_MEDIA_ROUTER=1', '-DUSE_PROPRIETARY_CODECS', '-DENABLE_PEPPER_CDMS', '-DENABLE_CONFIGURATION_POLICY', '-DENABLE_NOTIFICATIONS', '-DENABLE_TOPCHROME_MD=1', '-DENABLE_TASK_MANAGER=1', '-DENABLE_EXTENSIONS=1', '-DENABLE_PDF=1', '-DENABLE_PLUGIN_INSTALLATION=1', '-DENABLE_PLUGINS=1', '-DENABLE_SESSION_SERVICE=1', '-DENABLE_THEMES=1', '-DENABLE_AUTOFILL_DIALOG=1', '-DENABLE_PROD_WALLET_SERVICE=1', '-DENABLE_PRINTING=1', '-DENABLE_BASIC_PRINTING=1', '-DENABLE_PRINT_PREVIEW=1', '-DENABLE_SPELLCHECK=1', '-DUSE_BROWSER_SPELLCHECKER=1', '-DENABLE_CAPTIVE_PORTAL_DETECTION=1', '-DENABLE_APP_LIST=1', '-DENABLE_SETTINGS_APP=1', '-DENABLE_SUPERVISED_USERS=1', '-DENABLE_SERVICE_DISCOVERY=1', '-DENABLE_HANGOUT_SERVICES_EXTENSION=1', '-DV8_USE_EXTERNAL_STARTUP_DATA', '-DFULL_SAFE_BROWSING', '-DSAFE_BROWSING_CSD', '-DSAFE_BROWSING_DB_LOCAL', '-DUSE_LIBPCI=1', '-DUSE_OPENSSL=1', '-DUSE_OPENSSL_CERTS=1', '-D__STDC_CONSTANT_MACROS', '-D__STDC_FORMAT_MACROS', '-DBASE_IMPLEMENTATION', '-DSYSTEM_NATIVE_UTF8', '-DNACL_WINDOWS=0', '-DNACL_OSX=0', '-DNACL_LINUX=0', '-DNACL_ANDROID=0', '-DNACL_BUILD_ARCH=pnacl', '-I../out/Release/gen/tc_pnacl_newlib/include', '-I..', '-I../out/Release/gen', '-I..', '-DNDEBUG'] Linker options: ['-B../out/Release/gen/tc_pnacl_newlib/lib'] Traceback (most recent call last): File "../native_client/build/build_nexe.py", line 845, in CompileProcess output_queue.put((filename, build.Compile(filename))) File "../native_client/build/build_nexe.py", line 575, in Compile raise Error('FAILED with %d: %s' % (err, ' '.join(cmd_line))) Error: FAILED with 1: /b/build/goma/gomacc ../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-clang++ -c trace_event/malloc_dump_provider.cc -o ../out/Release/obj/base/base_nacl.gen/pnacl_newlib-pnacl/base_nacl/malloc_dump_provider_eeff7b85.o -MD -MF ../out/Release/obj/base/base_nacl.gen/pnacl_newlib-pnacl/base_nacl/malloc_dump_provider_eeff7b85.d -O2 -g -Wall -fdiagnostics-show-option -Werror -fno-strict-aliasing -Wno-unused-function -Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function -std=gnu++11 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1 -D_POSIX_C_SOURCE=199506 -D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DGOOGLE_CHROME_BUILD -DCR_CLANG_REVISION=263324-1 -DENABLE_RLZ -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_HANGOUT_SERVICES_EXTENSION=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DBASE_IMPLEMENTATION -DSYSTEM_NATIVE_UTF8 -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0 -DNACL_BUILD_ARCH=pnacl -I../out/Release/gen/tc_pnacl_newlib/include -I.. -I../out/Release/gen -I.. -DNDEBUG -std=gnu++0x -Wno-deprecated-register (...).
Message was sent while issue was closed.
https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1675183006/diff/180001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:217: // Insert/RemoveAllocation below will no-op if the register is torn down. On 2016/03/21 19:29:24, Primiano (traveling) wrote: > On 2016/03/15 19:36:03, Dmitry Skiba wrote: > > Hmm, why did you remove RemoveAllocatorDispatch() call from here? > Because, after chatting with chrisha, we decided to remove the > RemoveAllocatorDispatch API form the shim as it was hard to implement it in a > sane and thread-safe way. Will you also add checks to InsertAllocatorDispatch() to assert that a given AllocatorDispatch is not inserted twice? I.e. with a flag inside AllocatorDispatch for example. > > OnHeapProfilingEnabled(true) called for the second time will really mess > > allocator hooks by inserting g_allocator_hooks again. > OnHeapProfilingEnabled will never be called twice. The reason is that once you > start heap profiling, you cannot stop and start again, as you will unrecoverably > lose data (missing all the malloc/new that happen in the meantime). > The only supported case here is: turning heap profiling on and off only once, > and never again. > I added a comment to explain it explicitly here. What if I don't care about those allocations? I.e. I enabled heap profiling before doing something, to estimate impact of that something? For example, if I care about allocations during syncing, I would start profiling just before signing in. https://codereview.chromium.org/1675183006/diff/200001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (left): https://codereview.chromium.org/1675183006/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:69: // static This method (and others below) is not static anymore.
Message was sent while issue was closed.
FTR this was relanded in https://codereview.chromium.org/1831763003/ |
