Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163783003:
Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. (Closed)
|
Created:
4 years, 5 months ago by Sigurður Ásgeirsson Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@shim-default Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage.
This uses the generic allocator shim to hook into heap allocations.
When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation.
Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled.
BUG=644385
Committed: https://crrev.com/46e1b077586d5b91f9abb7a6a8f9eb6558a584bd
Cr-Commit-Position: refs/heads/master@{#417601}
Patch Set 1 #Patch Set 2 : Improve comments, fix cosmetic problems. #
Total comments: 34
Patch Set 3 : Address most comments from Chris and Primiano. #Patch Set 4 : Improve comments a bit. #Patch Set 5 : WIP: Push the size estimate function into AllocatorDispatch, rejig users and tests accordingly. #Patch Set 6 : Add the get_size_estimate function to other default impls. #Patch Set 7 : Fix a brain****, may even compile now. #
Total comments: 46
Patch Set 8 : Address Primiano's comments. #Patch Set 9 : Implement initial mocked shim testing, still to come: variation in the size function. #Patch Set 10 : Fix accounting semantics, fix testing, fix linkage in component builds, split tests into shim/integ… #Patch Set 11 : Fix speling[sic], add override specifier, fix unsigned literal comparisons. #Patch Set 12 : Make moar static to fix compilation error. #Patch Set 13 : Add a missing malloc.h include, fix the shim unittest after semantic change. #Patch Set 14 : Fix another @#$%&! literal signedness goof. #Patch Set 15 : Merge ToT and change ASSERT->EXPECT. #
Total comments: 43
Patch Set 16 : Address comments, add tests for overhead bytes and for realloc. #Patch Set 17 : Fix class static linkage problem. #Patch Set 18 : Fix the non-null realloc(..., 0) case. #Patch Set 19 : Add missing dispatch function and a test, pass right dispatch through to next level (sigh). #Patch Set 20 : Change Init implementation to return a bool status. ::Init is now also tested when the shim is disa… #
Total comments: 4
Patch Set 21 : Prevent clever compilers from optimizing out malloc/free pair in test. General cleanup. #Patch Set 22 : Change interface to Init/Enable/DisableForTesting, clean up tests. #Patch Set 23 : Fix memory leak found by ASAN bot. #
Total comments: 11
Patch Set 24 : Address Primiano's comments. #Patch Set 25 : Speling [sic] #Patch Set 26 : Pull enabled variable out of the #defines to avoid unused variable errors. #Patch Set 27 : Moar speling [sic]. #
Total comments: 6
Patch Set 28 : Address Nico's comments. #Messages
Total messages: 129 (99 generated)
siggi@chromium.org changed reviewers: + chrisha@chromium.org, primiano@chromium.org
Hey guys, here's what I'm wanting to hook up to the heap shim. This in turn would be invoked from base::debug::TaskAnnotator, and the per-task data then funneled into the task profiler. In addition to that, there's a specific task I want to profile for memory usage in the field, so I'd simply wrap that and UMA the results. Note that this is not ready for primetime as-is, as this is leaning on the Win heap to get the size of an allocation as-is. I think to make this clean I need to expand the shim with another function to expose that. I'm not super happy with the names of the class and the functions and the files and such, so please suggest moar better names. Otherwise WDYT? Siggi
https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:33: // instead allows querying the actual user size of the allocation with Did you mean: s/actual user size/actual size/ https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:34: // malloc_usable_size. I'm not sure how best to deal with this. Maybe by making allocated and freed track actual bytes of allocations, rather than user sizes? And make overhead track the exact or estimated difference between the actual allocations and user sizes, or be zero if unsupported? This would at least be consistent across all platforms. The cases: 1. You can know the user size and the allocation size. Do exact math. 2. You can know the user size and can't know the allocation size. Consistently estimate on alloc/free using the known user size, and have overhead be an estimate. 3. You can't know the user size, but can know the allocation size. Set overhead size to 0. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:161: } else { Shouldn't thread_usage_ always be non-null at this point? Guaranteed by GetOrCreateThreadUsage? https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:172: // our scope's max. This mechanism requires that scopes be properly nested, without overlap. Worth documenting somewhere? (Admittedly you'd have to go through various contortions to have this happen, but worth stating maybe?) https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:179: if (usage == nullptr) Can this even be null? https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... File base/debug/scoped_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:14: // - the number of alloc/free operations (realloc is one of each?) Makes sense to count one of each, unless the realloc was able to reuse the same exact address and simply extend it. In which case maybe it counts as neither? (I'm playing devil's advocate here, no strong opinion either way.) https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:16: // - the number of estimated (lower bound) bytes of heap overhead used. Will this always be a lower bound, or strictly an estimate? For some platforms we might not be able to do guarantee any valid lower bound other than '0', unless we know details of the heap implementation. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:37: // The maximal value of alloc_bytes - free_bytes seen for this thread. Very clear comments, great! https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:53: static void TearDownForTesting(); nit: protect the TearDownForTesting? A comment about the fact that apparently it is safe to recall Initialize if TearDown has since been called? Or is this not the case?
Some initial thoughts. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:34: // malloc_usable_size. I'm not sure how best to deal with this. So the problem here is that on other platforms malloc_usable_size() is terribly inconsistent. I don't remember the full details but I remember somebody reporting it to return just zero in lot of implementation (I think in Android). Which is the reason why in the heap profiler we keep track separately of the size of allocations (see base/trace_event/heap_profiler_allocation_register.h) together with other context stuff like stacktrace (when enabled) and such https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:43: size_t EstimateOverhead(size_t size) { this is going to be very tricky on tcmalloc (Linux/CrOS) where the overhead depends a lot on the thread patterns. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:136: g_thread_allocator_usage.Get(); Isn't this going to re-enter (either the lazy instance or the tls, very likely both)? I had some similar problems in base/trace_event/heap_profiler_allocation_context_tracker.cc you can take a look there for inspiration. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:140: CHECK(allocator_dispatch.next != nullptr); no need for this check, if this is null will crash 2 lines below anyways https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:146: next->alloc_function(next, sizeof(AllocatorUsage))); you could just use alloc_zero_initialized_function to avoid the memset below. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:148: usage_tls.Set(usage); so the problem here is that you are leaking one "usage" instace per thread. Which means that if something is creating and destroy threads you'll keep leaking this. In heap_profiler_allocation_context_tracker.cc I used ThreadLocalStorage::StaticSlot which allows to specify a thread-exit dtor for the object. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:161: } else { On 2016/07/21 14:23:31, chrisha (slow) wrote: > Shouldn't thread_usage_ always be non-null at this point? Guaranteed by > GetOrCreateThreadUsage? +1 https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:172: // our scope's max. On 2016/07/21 14:23:30, chrisha (slow) wrote: > This mechanism requires that scopes be properly nested, without overlap. Worth > documenting somewhere? > > (Admittedly you'd have to go through various contortions to have this happen, > but worth stating maybe?) maybe you can have a global Atomic32 g_scoped_heap_usage_enabled and a DCHECK to prevent overlapping usages?
Hey guys, I've addressed most or all comments and pushed the "GetSizeEstimate" function up to the shim dispatch. I think it's possible to support the semantics documented in the Scoped* class, falling back to only recording the allocated size and operations counts where the allocation size can't be retrieved. WDYT? This needs moar, better testing - probably best to expose the heap dispatch for testing and to test it against mocks with different capabilities? Siggi https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:33: // instead allows querying the actual user size of the allocation with On 2016/07/21 14:23:30, chrisha (slow) wrote: > Did you mean: s/actual user size/actual size/ Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:34: // malloc_usable_size. I'm not sure how best to deal with this. On 2016/07/21 14:23:31, chrisha (slow) wrote: > Maybe by making allocated and freed track actual bytes of allocations, rather > than user sizes? > > And make overhead track the exact or estimated difference between the actual > allocations and user sizes, or be zero if unsupported? > > This would at least be consistent across all platforms. > > The cases: > > 1. You can know the user size and the allocation size. Do exact math. > 2. You can know the user size and can't know the allocation size. Consistently > estimate on alloc/free using the known user size, and have overhead be an > estimate. > 3. You can't know the user size, but can know the allocation size. Set overhead > size to 0. Yeah - I think this'll be platform dependent enough that maybe it's best to implement it for Windows first, then refine it? https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:34: // malloc_usable_size. I'm not sure how best to deal with this. On 2016/07/21 15:59:20, Primiano Tucci wrote: > So the problem here is that on other platforms malloc_usable_size() is terribly > inconsistent. I don't remember the full details but I remember somebody > reporting it to return just zero in lot of implementation (I think in Android). > Which is the reason why in the heap profiler we keep track separately of the > size of allocations (see base/trace_event/heap_profiler_allocation_register.h) > together with other context stuff like stacktrace (when enabled) and such Side-tracking of individual allocs is a no-go for performance for this I think. Maybe the easiest thing to do is to expose a "feature flag probing" of some sort on the AllocatorDispatch, and to simply fail initialization if the underlying heap doesn't work? https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:43: size_t EstimateOverhead(size_t size) { On 2016/07/21 15:59:20, Primiano Tucci wrote: > this is going to be very tricky on tcmalloc (Linux/CrOS) where the overhead > depends a lot on the thread patterns. Yeah, it doesn't have to be very accurate though. Maybe the easiest thing to do is to delegate this also to the AllocatorDispatch? https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:136: g_thread_allocator_usage.Get(); On 2016/07/21 15:59:20, Primiano Tucci wrote: > Isn't this going to re-enter (either the lazy instance or the tls, very likely > both)? > I had some similar problems in > base/trace_event/heap_profiler_allocation_context_tracker.cc you can take a look > there for inspiration. I haven't seen it reenter, but obviously I haven't tried other platforms. I appropriated^Wadopted your implementation for TLS - thanks. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:140: CHECK(allocator_dispatch.next != nullptr); On 2016/07/21 15:59:20, Primiano Tucci wrote: > no need for this check, if this is null will crash 2 lines below anyways Acknowledged. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:146: next->alloc_function(next, sizeof(AllocatorUsage))); On 2016/07/21 15:59:20, Primiano Tucci wrote: > you could just use alloc_zero_initialized_function to avoid the memset below. Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:148: usage_tls.Set(usage); On 2016/07/21 15:59:20, Primiano Tucci wrote: > so the problem here is that you are leaking one "usage" instace per thread. > Which means that if something is creating and destroy threads you'll keep > leaking this. > > In heap_profiler_allocation_context_tracker.cc I used > ThreadLocalStorage::StaticSlot which allows to specify a thread-exit dtor for > the object. Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:161: } else { On 2016/07/21 14:23:31, chrisha (slow) wrote: > Shouldn't thread_usage_ always be non-null at this point? Guaranteed by > GetOrCreateThreadUsage? Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:161: } else { On 2016/07/21 15:59:20, Primiano Tucci wrote: > On 2016/07/21 14:23:31, chrisha (slow) wrote: > > Shouldn't thread_usage_ always be non-null at this point? Guaranteed by > > GetOrCreateThreadUsage? > > +1 Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:172: // our scope's max. On 2016/07/21 14:23:30, chrisha (slow) wrote: > This mechanism requires that scopes be properly nested, without overlap. Worth > documenting somewhere? > > (Admittedly you'd have to go through various contortions to have this happen, > but worth stating maybe?) Acknowledged. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:172: // our scope's max. On 2016/07/21 15:59:20, Primiano Tucci wrote: > On 2016/07/21 14:23:30, chrisha (slow) wrote: > > This mechanism requires that scopes be properly nested, without overlap. Worth > > documenting somewhere? > > > > (Admittedly you'd have to go through various contortions to have this happen, > > but worth stating maybe?) > > maybe you can have a global Atomic32 g_scoped_heap_usage_enabled and a DCHECK to > prevent overlapping usages? I'd prefer to just document this problem away, but another way to go would be to maintain the "current" scope in the TLS data, and to barf hard if ever there's a push/pop mismatch. This'd require the "prev" scope to be stored in the current scope as well. I added a comment on the class declaration. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.cc:179: if (usage == nullptr) On 2016/07/21 14:23:30, chrisha (slow) wrote: > Can this even be null? Done. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... File base/debug/scoped_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:14: // - the number of alloc/free operations (realloc is one of each?) On 2016/07/21 14:23:31, chrisha (slow) wrote: > Makes sense to count one of each, unless the realloc was able to reuse the same > exact address and simply extend it. In which case maybe it counts as neither? > > (I'm playing devil's advocate here, no strong opinion either way.) Dunno - I guess there are degenerate cases either way. In one instance you can get infinite churn with no operations as memory is freed and allocated in zero allocations/frees. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:16: // - the number of estimated (lower bound) bytes of heap overhead used. On 2016/07/21 14:23:31, chrisha (slow) wrote: > Will this always be a lower bound, or strictly an estimate? For some platforms > we might not be able to do guarantee any valid lower bound other than '0', > unless we know details of the heap implementation. Dunno - gotta see how that works out. I've dropped the lower bound comment for now. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:37: // The maximal value of alloc_bytes - free_bytes seen for this thread. On 2016/07/21 14:23:31, chrisha (slow) wrote: > Very clear comments, great! Acknowledged. https://codereview.chromium.org/2163783003/diff/20001/base/debug/scoped_heap_... base/debug/scoped_heap_usage.h:53: static void TearDownForTesting(); On 2016/07/21 14:23:31, chrisha (slow) wrote: > nit: protect the TearDownForTesting? Done. > > A comment about the fact that apparently it is safe to recall Initialize if > TearDown has since been called? Or is this not the case? Done.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
The CQ bit was unchecked by siggi@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Finally had time to look to this, sorry for the delay. Overall looks good an make sense. The major comment is that I don't see any reason for GetSizeEstimate to be part of function pointers of the shim chain. I think that if you make that just a global function everything becomes simpler here. Note that on Linux/Android (and perhaps mac too) I think you can just short-circuit this to malloc_usable_size() instead of depending on libc internals. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim.h:63: using GetSizeEstimateFn = size_t(const AllocatorDispatch* self, I don't think that this function should be part of the shim chain. If I read the code correctly, your intent here is not to "intercept" the GetSizeEstimate function, you just want to provide one implementation for each OS. I think the cleanest approach is adding a function to allocator_extensions.h, and in the .cc file have something like #if OS_LINUX && OS_ANDROID return malloc_usable_size(...) #elif OS_WINDOWS whatever incantation etc If you think to that, the GetEstimateSize function doesn't really depend on the shim, you are just leveraging it here because that happens to convenientely have the various if(OS/allocator). https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_glibc.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:44: void GlibcFree(const AllocatorDispatch*, void* address) { seems like you accidentally copy/pasted this twice. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:49: return __libc_usable_size(address); I don't think you need to go down into __libc internals (Also IIRC there is a Linux release bot that has a whitelist of dependent symbols and will complain if you try to add a new dependency). I think that both for Linux (tcmalloc or glibc) and Android, here you simply want to do : return malloc_usable_size(address) https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:23: #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) I think you can just restrict this #if to the Initialize and Teardown method and CHECK(false) in the #else case. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:50: size_t estimate = GetAllocSizeEstimate(next, ptr); yeah here I'd just call GetAllocSizeEstimate(ptr), declared in allocator_extensions (or whatever, as long as it's just a global function). No need to use the shim chain. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:106: if (ret != nullptr) I'd probably be a bit more conservative here and do if (ret != nullptr && size != 0) The reason being: the manpage for relloc seems to suggest that realloc(non_zero_address, 0 /*size*/) (which is essentially a free) can return non-null. Quoting from return value: "If size was equal to 0, either NULL or a pointer suitable to be passed to free() is returned" https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:113: if (address) small nit:y ou seem to mix if (address) with if (address != nullptr). I think both are fine, as long as long as you are consistent :) https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:118: // The dispatch for the heap interept used. typo: s/interept/intercept/ https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:133: const AllocatorDispatch* next = allocator_dispatch.next; given the fact that you have a sentinel I think you can safely just do: allocator_usage = new AllocatorUsage g_thread_....set . Even if you re-enter in the current hook you should be forwarded by the AllocFn. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:150: ScopedHeapUsage::ScopedHeapUsage() : thread_usage_(GetOrCreateThreadUsage()) { given that you initialize the other fields in the ctor body I'd probably just move this there for consistency? https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:158: thread_usage_->max_allocated_bytes) { out of curiosity are you caching thread_usage_ here? In other words, I wonder whether this would be more readable if you don't have the thread_usage_ field and here you just do: usage = GetOrCreateThreadUsage(); if (at_creation.max > usage.max) usage.max = at_creation.max; https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:23: // TODO(siggi): Should this perhaps leave the class declaration, cut the Yeah I think you really just need this in the Initialize/Teardown methods in the .cc file https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:27: class ScopedHeapUsage { you could probably use a threadchecker to check in DCHECK_IS_ON builds that this is effectively construted and destroyed on the same thread. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:27: class ScopedHeapUsage { After reading all this CL I think it would help if this class had "Thread" in the name, to make clear that this capture the heap stats for a given scope within one thread. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:29: struct AllocatorUsage { Similarly I'd put thread in the name here as well. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:32: // The cumulative number of allocated bytes. Where available, this is nit: newlines here and below between comments and the previous field https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage_unittest.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage_unittest.cc:36: EXPECT_EQ(u1.alloc_ops, u2.alloc_ops); I'd move these expect statements below., after u3 If for any reason EXPECT_EQ does some malloc (to lazily initiailize something) this test will break in very puzzling ways. https://codereview.chromium.org/2163783003/diff/200001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:77: size_t HookGetSizeEstimate(const AllocatorDispatch* self, void* address) { yeah you don't really need to touch this at all if you use a global function.
comments after our vc, see if it makes sense. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim.h:63: using GetSizeEstimateFn = size_t(const AllocatorDispatch* self, On 2016/08/24 14:11:46, Primiano Tucci wrote: > I don't think that this function should be part of the shim chain. If I read the > code correctly, your intent here is not to "intercept" the GetSizeEstimate > function, you just want to provide one implementation for each OS. > > I think the cleanest approach is adding a function to allocator_extensions.h, > and in the .cc file have something like > #if OS_LINUX && OS_ANDROID > return malloc_usable_size(...) > #elif OS_WINDOWS > whatever incantation > etc > > If you think to that, the GetEstimateSize function doesn't really depend on the > shim, you are just leveraging it here because that happens to convenientely have > the various if(OS/allocator). siggi@ and I had a chat offline. siggi@ explained me that in the near future there is the desire to use the shim to provide some kind of statistically-alternate-heap. In that view, makes sense for the GetSizeEstimate function to be shimmed, as that implementation will become dependent on the presence of the "alternate heap" shim layers. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_glibc.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:17: size_t __malloc_usable_size(void* ptr); not sure you ned this fwd declaration https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:49: return __libc_usable_size(address); On 2016/08/24 14:11:46, Primiano Tucci wrote: > I don't think you need to go down into __libc internals (Also IIRC there is a > Linux release bot that has a whitelist of dependent symbols and will complain if > you try to add a new dependency). > I think that both for Linux (tcmalloc or glibc) and Android, here you simply > want to do : > > return malloc_usable_size(address) I think you mean __malloc_usable_size here. The natural question that comes here is: should we also wrap and hook calls to malloc_usable_size and route them to this function ? That would guarantee that if code inside of chrome calls that, they get trapped by the shim and we can provide our own implementation. Not sure how much we care about that. I think the realistic use case here is: if in the near future we provide an alternative heap implementation and chromium code calls malloc_usable_size, will it crash if we ask malloc_usable_some(some_Address_that_was_intercepted_by_the_shim?) Maybe we should do that, in a later CL, for consistency and to cover ourself from this case. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:50: return 0; I can take care of this later. The trick here is to add malloc_usable_size to the wrapped symbols in base/allocator/BUILD.gn, which will give a __real_malloc_usable_size here (but requires the shim to expose a symbol to route the __wrap_malloc_usable_size into). Fine to leave it with a TODO for the moment. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:37: size_t GetAllocSizeEstimate(const AllocatorDispatch* next, void* ptr) { So at this point, if we shim GetAllocSize, there should be a global entry point declared/defined in allocator_shim.h/cc, e.g. BASE_EXPORT size_t GetAllocSizeEstimate(void* ptr) next to UncheckedAlloc, and that should be implemented like: { const allocator::AllocatorDispatch* const chain_head = GetChainHead(); return chain_head->get_alloc_size_function(chain_head, ptr); }
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:320001) has been deleted
Patchset #11 (id:360001) has been deleted
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Chris - can I ask you to give this the once-over? I'm still trying to get this to compile and pass tests across all our configs and platforms, but I think I'm down to the nitty-gritty now. Primiano, I'll ask you to give this a look after Chris is done shredding me - if that ok? https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim.h:63: using GetSizeEstimateFn = size_t(const AllocatorDispatch* self, On 2016/08/24 14:11:46, Primiano Tucci wrote: > I don't think that this function should be part of the shim chain. If I read the > code correctly, your intent here is not to "intercept" the GetSizeEstimate > function, you just want to provide one implementation for each OS. > > I think the cleanest approach is adding a function to allocator_extensions.h, > and in the .cc file have something like > #if OS_LINUX && OS_ANDROID > return malloc_usable_size(...) > #elif OS_WINDOWS > whatever incantation > etc > > If you think to that, the GetEstimateSize function doesn't really depend on the > shim, you are just leveraging it here because that happens to convenientely have > the various if(OS/allocator). Acknowledged. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim.h:63: using GetSizeEstimateFn = size_t(const AllocatorDispatch* self, On 2016/08/24 15:28:46, Primiano Tucci wrote: > On 2016/08/24 14:11:46, Primiano Tucci wrote: > > I don't think that this function should be part of the shim chain. If I read > the > > code correctly, your intent here is not to "intercept" the GetSizeEstimate > > function, you just want to provide one implementation for each OS. > > > > I think the cleanest approach is adding a function to allocator_extensions.h, > > and in the .cc file have something like > > #if OS_LINUX && OS_ANDROID > > return malloc_usable_size(...) > > #elif OS_WINDOWS > > whatever incantation > > etc > > > > If you think to that, the GetEstimateSize function doesn't really depend on > the > > shim, you are just leveraging it here because that happens to convenientely > have > > the various if(OS/allocator). > > siggi@ and I had a chat offline. siggi@ explained me that in the near future > there is the desire to use the shim to provide some kind of > statistically-alternate-heap. In that view, makes sense for the GetSizeEstimate > function to be shimmed, as that implementation will become dependent on the > presence of the "alternate heap" shim layers. Yeah - it would be nice to fully abstract the notion of a heap with the shim, perhaps to the point of e.g. getting the aggregate memory usage for tracing through the shim. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_glibc.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:17: size_t __malloc_usable_size(void* ptr); On 2016/08/24 15:28:46, Primiano Tucci wrote: > not sure you ned this fwd declaration Done. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:44: void GlibcFree(const AllocatorDispatch*, void* address) { On 2016/08/24 14:11:46, Primiano Tucci wrote: > seems like you accidentally copy/pasted this twice. Ooops, I wonder how this even compiled. Fixed :/. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:49: return __libc_usable_size(address); On 2016/08/24 14:11:46, Primiano Tucci wrote: > I don't think you need to go down into __libc internals (Also IIRC there is a > Linux release bot that has a whitelist of dependent symbols and will complain if > you try to add a new dependency). > I think that both for Linux (tcmalloc or glibc) and Android, here you simply > want to do : > > return malloc_usable_size(address) Done. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_glibc.cc:49: return __libc_usable_size(address); On 2016/08/24 15:28:46, Primiano Tucci wrote: > On 2016/08/24 14:11:46, Primiano Tucci wrote: > > I don't think you need to go down into __libc internals (Also IIRC there is a > > Linux release bot that has a whitelist of dependent symbols and will complain > if > > you try to add a new dependency). > > I think that both for Linux (tcmalloc or glibc) and Android, here you simply > > want to do : > > > > return malloc_usable_size(address) > > I think you mean __malloc_usable_size here. > > The natural question that comes here is: should we also wrap and hook calls to > malloc_usable_size and route them to this function ? > That would guarantee that if code inside of chrome calls that, they get trapped > by the shim and we can provide our own implementation. > Not sure how much we care about that. I think the realistic use case here is: if > in the near future we provide an alternative heap implementation and chromium > code calls malloc_usable_size, will it crash if we ask > malloc_usable_some(some_Address_that_was_intercepted_by_the_shim?) > Maybe we should do that, in a later CL, for consistency and to cover ourself > from this case. Thanks, added a TODO. https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:50: return 0; On 2016/08/24 15:28:46, Primiano Tucci wrote: > I can take care of this later. > The trick here is to add malloc_usable_size to the wrapped symbols in > base/allocator/BUILD.gn, which will give a __real_malloc_usable_size here (but > requires the shim to expose a symbol to route the __wrap_malloc_usable_size > into). > Fine to leave it with a TODO for the moment. Thanks, I've left you a TODO. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:23: #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) On 2016/08/24 14:11:46, Primiano Tucci wrote: > I think you can just restrict this #if to the Initialize and Teardown method and > CHECK(false) in the #else case. Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:37: size_t GetAllocSizeEstimate(const AllocatorDispatch* next, void* ptr) { On 2016/08/24 15:28:47, Primiano Tucci wrote: > So at this point, if we shim GetAllocSize, there should be a global entry point > declared/defined in allocator_shim.h/cc, e.g. > > BASE_EXPORT size_t GetAllocSizeEstimate(void* ptr) next to UncheckedAlloc, and > that should be implemented like: > > { > const allocator::AllocatorDispatch* const chain_head = GetChainHead(); > return chain_head->get_alloc_size_function(chain_head, ptr); > } I can add this if you like, but I don't agree with your reasoning in the point below. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:50: size_t estimate = GetAllocSizeEstimate(next, ptr); On 2016/08/24 14:11:46, Primiano Tucci wrote: > yeah here I'd just call GetAllocSizeEstimate(ptr), declared in > allocator_extensions (or whatever, as long as it's just a global function). No > need to use the shim chain. As-is, I can test the functionality of this shim in isolation against a mocked "next" dispatch implementation. If this starts calling on global functions that refer to the shim chain head, then that's no longer possible. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:106: if (ret != nullptr) On 2016/08/24 14:11:46, Primiano Tucci wrote: > I'd probably be a bit more conservative here and do > if (ret != nullptr && size != 0) > > The reason being: the manpage for relloc seems to suggest that > realloc(non_zero_address, 0 /*size*/) (which is essentially a free) can return > non-null. > Quoting from return value: "If size was equal to 0, either NULL or a pointer > suitable to be passed to free() is returned" Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:113: if (address) On 2016/08/24 14:11:46, Primiano Tucci wrote: > small nit:y ou seem to mix if (address) with if (address != nullptr). > I think both are fine, as long as long as you are consistent :) Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:118: // The dispatch for the heap interept used. On 2016/08/24 14:11:46, Primiano Tucci wrote: > typo: s/interept/intercept/ Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:133: const AllocatorDispatch* next = allocator_dispatch.next; On 2016/08/24 14:11:46, Primiano Tucci wrote: > given the fact that you have a sentinel I think you can safely just do: > > allocator_usage = new AllocatorUsage > g_thread_....set . > > Even if you re-enter in the current hook you should be forwarded by the AllocFn. Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:150: ScopedHeapUsage::ScopedHeapUsage() : thread_usage_(GetOrCreateThreadUsage()) { On 2016/08/24 14:11:46, Primiano Tucci wrote: > given that you initialize the other fields in the ctor body I'd probably just > move this there for consistency? Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.cc:158: thread_usage_->max_allocated_bytes) { On 2016/08/24 14:11:46, Primiano Tucci wrote: > out of curiosity are you caching thread_usage_ here? > In other words, I wonder whether this would be more readable if you don't have > the thread_usage_ field and here you just do: > > usage = GetOrCreateThreadUsage(); > if (at_creation.max > usage.max) usage.max = at_creation.max; Done. This does require two TLS lookups per instance, which should be no biggie. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:23: // TODO(siggi): Should this perhaps leave the class declaration, cut the On 2016/08/24 14:11:46, Primiano Tucci wrote: > Yeah I think you really just need this in the Initialize/Teardown methods in the > .cc file Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:27: class ScopedHeapUsage { On 2016/08/24 14:11:46, Primiano Tucci wrote: > After reading all this CL I think it would help if this class had "Thread" in > the name, to make clear that this capture the heap stats for a given scope > within one thread. Good idea, done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:27: class ScopedHeapUsage { On 2016/08/24 14:11:46, Primiano Tucci wrote: > you could probably use a threadchecker to check in DCHECK_IS_ON builds that this > is effectively construted and destroyed on the same thread. Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:29: struct AllocatorUsage { On 2016/08/24 14:11:46, Primiano Tucci wrote: > Similarly I'd put thread in the name here as well. Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage.h:32: // The cumulative number of allocated bytes. Where available, this is On 2016/08/24 14:11:46, Primiano Tucci wrote: > nit: newlines here and below between comments and the previous field Done. https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... File base/debug/scoped_heap_usage_unittest.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/debug/scoped_heap... base/debug/scoped_heap_usage_unittest.cc:36: EXPECT_EQ(u1.alloc_ops, u2.alloc_ops); On 2016/08/24 14:11:46, Primiano Tucci wrote: > I'd move these expect statements below., after u3 > If for any reason EXPECT_EQ does some malloc (to lazily initiailize something) > this test will break in very puzzling ways. Yeah, I'm going to rewrite this test to test the intercept in two parts: 1. it's chained in from a malloc or the like. 2. invoke on the dispatch functions directly with a mocked "next" dispatch to simulate various behaviors in the underlying get_size function. https://codereview.chromium.org/2163783003/diff/200001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2163783003/diff/200001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:77: size_t HookGetSizeEstimate(const AllocatorDispatch* self, void* address) { On 2016/08/24 14:11:46, Primiano Tucci wrote: > yeah you don't really need to touch this at all if you use a global function. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim.h:62: // allocation. What should implementations do when this isn't possible? Is there a way to communicate that via this API? Or is it assumed that it will always be possible? A note that the estimate should be consistent/deterministic? ie. return the same value for the same inputs? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:20: #include "base/threading/thread_local_storage.h" Wonky include order? Is this necessary for some reason? If so, will likely require some LINT statements? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:54: usage->alloc_overhead_bytes += size - estimate; estimate includes overhead, and size doesn't, no? Should alloc_overhead_bytes by += estimate - size? In fact, shouldn't there be a DCHECK_LE(size, estimate)? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:71: size_t estimate = GetAllocSizeEstimate(next, ptr); I'd find a comment about GetAllocSizeEstimate returning 0 when not supported as useful here, and the fact that free_bytes will remain zero in that case? (But then again, I'm a little slow.) https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; (Unfamiliar with the impl details.) You don't need to create versions of each function, the shim will automatically forward them to the next dispatch if there is one? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:137: allocator_usage = new ScopedThreadHeapUsage::ThreadAllocatorUsage; Do we need this dance? Or could you simply pass in the |next| pointer to this function, and directly call the allocator below you? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:149: // for testing. Expose a static EnsureTLSInitializedForTesting, and manually call it there in your fixture? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:155: // Reset the stats for our current scope. A clarifying comment would help here: The TLS info is now tracking the deltas from this point forward. The values will be restored to be globally meaningful when this object is destroyed. (I'm sure you can write this better than me.) https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:195: void ScopedThreadHeapUsage::EnsureTLSInitalized() { Initialized* (here and elsewhere) https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:21: // - the number of alloc/free operations (realloc is one of each?) Answer the question before landing this? Is realloc once of each? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:40: uint64_t alloc_overhead_bytes; Is this ever available? There's no way to get this from the allocator shim API? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage_unittest.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:24: // A fixture class that aloows testing the AllocatorDispatch associated with allows* https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:301: // While the inner scope saw only the inner net outstanding allocaiton size, allocation* https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:342: // Verify that at least the one free operation abobve was recorded. above*
Proactive-LGTM to kill timezone latency. Overall looks good % remaining Chris comments. Can do another pass on Monday if this is not landed by then, otherwise feels to me in the right shape anyways. https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim.h:62: // allocation. On 2016/09/01 20:29:17, chrisha (slow) wrote: > What should implementations do when this isn't possible? Is there a way to > communicate that via this API? Or is it assumed that it will always be possible? Maybe just return 0? IIRC that is the same pattern on POSIX with malloc_usable_size, where some malloc implementation just return always zero because they can't be bothered with implementing that :) https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:50: // the like. yup I can do this in a followup,it's quite straightforward. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; On 2016/09/01 20:29:17, chrisha (slow) wrote: > (Unfamiliar with the impl details.) > > You don't need to create versions of each function, the shim will automatically > forward them to the next dispatch if there is one? Nope, he actually has to do that. Doing what you say would have required an "if(null) use .next" in the fastpath of the standard case that I wanted to avoid. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:137: allocator_usage = new ScopedThreadHeapUsage::ThreadAllocatorUsage; On 2016/09/01 20:29:17, chrisha (slow) wrote: > Do we need this dance? Or could you simply pass in the |next| pointer to this > function, and directly call the allocator below you? I thinkthe problem is that you never know if in turn creating the ThreadAllocatorUsage does other mallocs under the hoods (on linux some pthread functions do require malloc). This feels a safer version. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:149: // for testing. On 2016/09/01 20:29:17, chrisha (slow) wrote: > Expose a static EnsureTLSInitializedForTesting, and manually call it there in > your fixture? +1 https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:21: // - the number of alloc/free operations (realloc is one of each?) On 2016/09/01 20:29:18, chrisha (slow) wrote: > Answer the question before landing this? Is realloc once of each? I think conceptually is fine counting realloc as one each because you don't know what happens for real under the hoods, but very likely growing the allocation will require a memmove(). All this % the fake reallocs, like realloc(nullptr, 42) which means really malloc, and realloc(ptr, 0) which means really free. Which should be just counted as such.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks guys - I've addressed all comments, aside from the TLS initialization one. I think maybe it's cleanest to always allow/require ::Initialize before instantiating a Scoped* instance, but to return a bool status that's false on "failure", e.g. when the shim is not available? https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim.h:62: // allocation. On 2016/09/01 20:29:17, chrisha (slow) wrote: > What should implementations do when this isn't possible? Is there a way to > communicate that via this API? Or is it assumed that it will always be possible? > > A note that the estimate should be consistent/deterministic? ie. return the same > value for the same inputs? Added mention that zero returns means "dunno". https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim.h:62: // allocation. On 2016/09/02 17:31:20, Primiano Tucci wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > What should implementations do when this isn't possible? Is there a way to > > communicate that via this API? Or is it assumed that it will always be > possible? > > Maybe just return 0? IIRC that is the same pattern on POSIX with > malloc_usable_size, where some malloc implementation just return always zero > because they can't be bothered with implementing that :) Done. https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/allocator/allocat... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:50: // the like. On 2016/09/02 17:31:20, Primiano Tucci wrote: > yup I can do this in a followup,it's quite straightforward. Thanks. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:20: #include "base/threading/thread_local_storage.h" On 2016/09/01 20:29:17, chrisha (slow) wrote: > Wonky include order? Is this necessary for some reason? If so, will likely > require some LINT statements? The OS defines require build_config, but the rest is just me being messy. fixed. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:54: usage->alloc_overhead_bytes += size - estimate; On 2016/09/01 20:29:17, chrisha (slow) wrote: > estimate includes overhead, and size doesn't, no? Should alloc_overhead_bytes by > += estimate - size? > > In fact, shouldn't there be a DCHECK_LE(size, estimate)? Too right, adding a test. I don't think DCHECK will be wise here, as that'll reenter the allocator :/ https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:71: size_t estimate = GetAllocSizeEstimate(next, ptr); On 2016/09/01 20:29:17, chrisha (slow) wrote: > I'd find a comment about GetAllocSizeEstimate returning 0 when not supported as > useful here, and the fact that free_bytes will remain zero in that case? > > (But then again, I'm a little slow.) Added a class comment 'splaining this. Maybe more useful at interface than in implementation? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; On 2016/09/02 17:31:20, Primiano Tucci wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > (Unfamiliar with the impl details.) > > > > You don't need to create versions of each function, the shim will > automatically > > forward them to the next dispatch if there is one? > > Nope, he actually has to do that. > Doing what you say would have required an "if(null) use .next" in the fastpath > of the standard case that I wanted to avoid. Acknowledged. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; On 2016/09/01 20:29:17, chrisha (slow) wrote: > (Unfamiliar with the impl details.) > > You don't need to create versions of each function, the shim will automatically > forward them to the next dispatch if there is one? Nope, you don't slow the critical common path with loops and shit to save a programmer a couple of keystrokes. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:137: allocator_usage = new ScopedThreadHeapUsage::ThreadAllocatorUsage; On 2016/09/01 20:29:17, chrisha (slow) wrote: > Do we need this dance? Or could you simply pass in the |next| pointer to this > function, and directly call the allocator below you? Yeah, as Primiano pointed out, if this ever breaks, it'll be much more costly to debug it than it is to prevent breakage due to reentrancy. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:137: allocator_usage = new ScopedThreadHeapUsage::ThreadAllocatorUsage; On 2016/09/02 17:31:20, Primiano Tucci wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > Do we need this dance? Or could you simply pass in the |next| pointer to this > > function, and directly call the allocator below you? > > I thinkthe problem is that you never know if in turn creating the > ThreadAllocatorUsage does other mallocs under the hoods (on linux some pthread > functions do require malloc). > This feels a safer version. Acknowledged. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:149: // for testing. On 2016/09/01 20:29:17, chrisha (slow) wrote: > Expose a static EnsureTLSInitializedForTesting, and manually call it there in > your fixture? Acknowledged. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:149: // for testing. On 2016/09/02 17:31:20, Primiano Tucci wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > Expose a static EnsureTLSInitializedForTesting, and manually call it there in > > your fixture? > > +1 I'm actually of two minds here. The TLS initialization is orthogonal to insertion of the heap shim. However, as documented, it's not permitted to initialize the class in the absence of the heap shim (build flag). I'm tempted to have it always allowed to call initialization function, and set up the TLS there, but to return true or false depending on whether the heap shimming is available? That way the init code can be without define conditionals, as can usage of the scoped classes. WDYT? https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:155: // Reset the stats for our current scope. On 2016/09/01 20:29:17, chrisha (slow) wrote: > A clarifying comment would help here: > > The TLS info is now tracking the deltas from this point forward. The values will > be restored to be globally meaningful when this object is destroyed. > > (I'm sure you can write this better than me.) Done. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:195: void ScopedThreadHeapUsage::EnsureTLSInitalized() { On 2016/09/01 20:29:17, chrisha (slow) wrote: > Initialized* (here and elsewhere) Whoops - thanks, fixed. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:21: // - the number of alloc/free operations (realloc is one of each?) On 2016/09/01 20:29:18, chrisha (slow) wrote: > Answer the question before landing this? Is realloc once of each? Done. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:21: // - the number of alloc/free operations (realloc is one of each?) On 2016/09/02 17:31:20, Primiano Tucci wrote: > On 2016/09/01 20:29:18, chrisha (slow) wrote: > > Answer the question before landing this? Is realloc once of each? > > I think conceptually is fine counting realloc as one each because you don't know > what happens for real under the hoods, but very likely growing the allocation > will require a memmove(). > All this % the fake reallocs, like realloc(nullptr, 42) which means really > malloc, and realloc(ptr, 0) which means really free. Which should be just > counted as such. Done. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:40: uint64_t alloc_overhead_bytes; On 2016/09/01 20:29:17, chrisha (slow) wrote: > Is this ever available? There's no way to get this from the allocator shim API? It's the difference between the requested size and the estimate on alloc. By keeping the usage tally in estimate bytes (when available), you can at least sorta, kinda get the average overhead per alloc in a scope. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage_unittest.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:24: // A fixture class that aloows testing the AllocatorDispatch associated with On 2016/09/01 20:29:18, chrisha (slow) wrote: > allows* Done. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:301: // While the inner scope saw only the inner net outstanding allocaiton size, On 2016/09/01 20:29:18, chrisha (slow) wrote: > allocation* Done. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage_unittest.cc:342: // Verify that at least the one free operation abobve was recorded. On 2016/09/01 20:29:18, chrisha (slow) wrote: > above* Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; On 2016/09/06 14:58:53, Sigurður Ásgeirsson wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > (Unfamiliar with the impl details.) > > > > You don't need to create versions of each function, the shim will > automatically > > forward them to the next dispatch if there is one? > > Nope, you don't slow the critical common path with loops and shit to save a > programmer a couple of Maybe I should have been more clear. You appear to be missing a pointer for the GetSizeEstimate function.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks - now less incorrect and moar tested. https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:124: nullptr}; On 2016/09/06 15:51:43, chrisha (slow) wrote: > On 2016/09/06 14:58:53, Sigurður Ásgeirsson wrote: > > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > > (Unfamiliar with the impl details.) > > > > > > You don't need to create versions of each function, the shim will > > automatically > > > forward them to the next dispatch if there is one? > > > > Nope, you don't slow the critical common path with loops and shit to save a > > programmer a couple of > > Maybe I should have been more clear. You appear to be missing a pointer for the > GetSizeEstimate function. Holy Cannoli batman, you're too right. I'm upside down backwards with missing breeches! Adding a test!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
K, so I've changed my mind on the ::Init function once again. Here's what I'm thinking and why: This function wants to take a "bool intercept_heap" and return a boolean result. Alternatively the initialization should be split into two functions, one for the TLS scaffolding and the other to hook in the shim. Either way, this will allow shipping Chrome with instrumentation in place but disabled - at practically no runtime cost - but to enable the intercept under a command line flag or a finch experiment. WDYT? On Tue, Sep 6, 2016 at 12:25 PM <siggi@chromium.org> wrote: > Thanks - now less incorrect and moar tested. > > > > > https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... > File base/debug/scoped_thread_heap_usage.cc (right): > > > https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... > base/debug/scoped_thread_heap_usage.cc:124: nullptr}; > On 2016/09/06 15:51:43, chrisha (slow) wrote: > > On 2016/09/06 14:58:53, Sigurður Ásgeirsson wrote: > > > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > > > (Unfamiliar with the impl details.) > > > > > > > > You don't need to create versions of each function, the shim will > > > automatically > > > > forward them to the next dispatch if there is one? > > > > > > Nope, you don't slow the critical common path with loops and shit to > save a > > > programmer a couple of > > > > Maybe I should have been more clear. You appear to be missing a > pointer for the > > GetSizeEstimate function. > > Holy Cannoli batman, you're too right. I'm upside down backwards with > missing breeches! Adding a test! > > https://codereview.chromium.org/2163783003/ > -- 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.
Description was changed from ========== Implement a ScopedHeapUsage class to allow profiling heap usage. BUG= ========== to ========== Implement a ScopedHeapUsage class to allow profiling heap usage. BUG=644385 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> This function wants to take a "bool intercept_heap" and return a boolean result. Alternatively the initialization should be split into two functions, one for the TLS scaffolding and the other to hook in the shim. So depends on what really happens if you don't init the TLS and try to use the class? Will it crash or not work? if it crashes, it's probably safer to have a separate Initialize (which just does the tls stuff) and a static bool EnableHeapTracking(bool enabled) method to turning on and off Having a separate TLS initializer is not something I've never seen before. Actually on some platforms (Linux) you need to initialize the TLS as early as possible to make sure that you get in the "slot fastpath" which doesn't require malloc for the lookup. OKAY, dejavu, I've already seen this, precisely. See https://codereview.chromium.org/2023133003/, I guess you want to place your TLS initialize just next to that other one. There is precedent :) https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:159: memset(usage, 0, sizeof(*usage)); maybe add also a static_assert(is_trivially_constructible... (from base/template_util.h) to make sure that this memset remains safe. https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:190: EnsureTLSInitialized(); you can probaby add a bool g_initialized with a CHECK() against dual initialization. This is //base and people will abuse and misuse this in any possible way :P Also you could DCHECK(g_initialized) in the ctor of each instance for helping debugging.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/06 19:33:12, Primiano Tucci wrote: > > This function wants to take a "bool intercept_heap" and return a boolean > result. Alternatively the initialization should be split into two > functions, one for the TLS scaffolding and the other to hook in the shim. > > So depends on what really happens if you don't init the TLS and try to use the > class? Will it crash or not work? I dunno about other platforms, but on Windows you'll end up stomping someone else's TLS slot. Unfortunately this will typically not crash right away :/. >if it crashes, it's probably safer to have a > separate Initialize (which just does the tls stuff) and a static bool > EnableHeapTracking(bool enabled) method to turning on and off > Yups, I like that - this is nice and transparent. A change to Initialize/EnableHeapTracking and DisableHeapTrackingForTesting coming up. > Having a separate TLS initializer is not something I've never seen before. > Actually on some platforms (Linux) you need to initialize the TLS as early as > possible to make sure that you get in the "slot fastpath" which doesn't require > malloc for the lookup. Actually I think so long as the TLS slot is initialized before the shim is inserted, everything should be fine? > > OKAY, dejavu, I've already seen this, precisely. > See https://codereview.chromium.org/2023133003/, I guess you want to place your > TLS initialize just next to that other one. There is precedent :) > Thanks, let's hope this won't be needed :).
Description was changed from ========== Implement a ScopedHeapUsage class to allow profiling heap usage. BUG=644385 ========== to ========== Implement a ScopedThreadHeapUsage class to allow profiling heap usage. This uses the generic allocator shim to hook into heap allocations. BUG=644385 ==========
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK guys, I think this is finally ready for submission - one last look? Thanks for your patience, and primiano, thanks for your help figuring out that the test failures were due to the compiler optimizing out the malloc/free calls :/. https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:159: memset(usage, 0, sizeof(*usage)); On 2016/09/06 19:33:12, Primiano Tucci wrote: > maybe add also a > static_assert(is_trivially_constructible... (from base/template_util.h) to make > sure that this memset remains safe. Good idea. I added std::is_pod, which should do the trick? https://codereview.chromium.org/2163783003/diff/560001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:190: EnsureTLSInitialized(); On 2016/09/06 19:33:12, Primiano Tucci wrote: > you can probaby add a bool g_initialized with a CHECK() against dual > initialization. > This is //base and people will abuse and misuse this in any possible way :P > Also you could DCHECK(g_initialized) in the ctor of each instance for helping > debugging. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
great. still LGTM, just some final comments about checks. https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:18: #include <malloc/malloc.h> the order of includes seems a bit weird here. but as long as the presubmit check is happy I'm fine. IIRC should be #include "corresponding_header.h" #include <cincludes.h> #include <cppincludes> #include "chrome/stuff.h" https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:40: if (ptr == nullptr || !next->get_size_estimate_function) ditto about get_size_estimate_function should never be null (nor any other shim function). I think this should be just if (ptr == nullptr) https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:202: } i'd probably add an else NOTREACHED() (or just make it a check). If somebody is calling Initialize twice feels like something is wrong (or am I missing something)? https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:207: base::allocator::InsertAllocatorDispatch(&allocator_dispatch); this one also I'd guard with a (D)CHECK against double enabling. The shim will break apart if you insert twice :) https://codereview.chromium.org/2163783003/diff/620001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2163783003/diff/620001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:82: if (!next->get_size_estimate_function) not sure we need this level of protection here right? it should never happen that the ptr is null. if not supported this should have a return 0 implementation, but the elements of the chain shouldn't be bothered to make this check
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 siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
siggi@chromium.org changed reviewers: + thakis@chromium.org
Thanks - comments addressed. Chris? Nico for owners pretty please? https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:18: #include <malloc/malloc.h> On 2016/09/07 17:53:38, Primiano Tucci wrote: > the order of includes seems a bit weird here. but as long as the presubmit check > is happy I'm fine. > > IIRC should be > > #include "corresponding_header.h" > > #include <cincludes.h> > > #include <cppincludes> > > #include "chrome/stuff.h" The OS defines come from build/build_config.h - hence this order. https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:40: if (ptr == nullptr || !next->get_size_estimate_function) On 2016/09/07 17:53:38, Primiano Tucci wrote: > ditto about get_size_estimate_function should never be null (nor any other shim > function). I think this should be just if (ptr == nullptr) Done. https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:202: } On 2016/09/07 17:53:38, Primiano Tucci wrote: > i'd probably add an else NOTREACHED() (or just make it a check). If somebody is > calling Initialize twice feels like something is wrong (or am I missing > something)? This'll happen in tests - I don't think it's possible to un-init the TLS stuff? https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:207: base::allocator::InsertAllocatorDispatch(&allocator_dispatch); On 2016/09/07 17:53:38, Primiano Tucci wrote: > this one also I'd guard with a (D)CHECK against double enabling. The shim will > break apart if you insert twice :) Done. https://codereview.chromium.org/2163783003/diff/620001/base/trace_event/mallo... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/2163783003/diff/620001/base/trace_event/mallo... base/trace_event/malloc_dump_provider.cc:82: if (!next->get_size_estimate_function) On 2016/09/07 17:53:38, Primiano Tucci wrote: > not sure we need this level of protection here right? it should never happen > that the ptr is null. if not supported this should have a return 0 > implementation, but the elements of the chain shouldn't be bothered to make this > check Done.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/620001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:202: } On 2016/09/07 18:35:35, Sigurður Ásgeirsson wrote: > On 2016/09/07 17:53:38, Primiano Tucci wrote: > > i'd probably add an else NOTREACHED() (or just make it a check). If somebody > is > > calling Initialize twice feels like something is wrong (or am I missing > > something)? > > This'll happen in tests - I don't think it's possible to un-init the TLS stuff? ah yes you are right. fine as it is then.
Nothing more to add from me. lgtm! https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/460001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:71: size_t estimate = GetAllocSizeEstimate(next, ptr); On 2016/09/06 14:58:53, Sigurður Ásgeirsson wrote: > On 2016/09/01 20:29:17, chrisha (slow) wrote: > > I'd find a comment about GetAllocSizeEstimate returning 0 when not supported > as > > useful here, and the fact that free_bytes will remain zero in that case? > > > > (But then again, I'm a little slow.) > > Added a class comment 'splaining this. Maybe more useful at interface than in > implementation? sgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I haven't looked at the code, but... I want a bit more detail in the CL description. The first line should match the issue title, and the rest can move to the body. The main thing I want to know is whether the measurements are per-thread or per-process. Some estimates on the overhead when this is enabled and when it isn't would also be appreciated.
Description was changed from ========== Implement a ScopedThreadHeapUsage class to allow profiling heap usage. This uses the generic allocator shim to hook into heap allocations. BUG=644385 ========== to ========== Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. This uses the generic allocator shim to hook into heap allocations. When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation. Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled. BUG=644385 ==========
Thanks, updated the issue title and body. The ScopedThreadHeapUsage class name was intended to be transparent, and IMHO it wouldn't be much use to measure program-wide heap usage over a scope? Hence, the ScopedThreadHeapUsage class measures the heap usage of a thread, over a scope. On Wed, Sep 7, 2016 at 5:40 PM <brucedawson@chromium.org> wrote: > I haven't looked at the code, but... > > I want a bit more detail in the CL description. The first line should > match the > issue title, and the rest can move to the body. > > The main thing I want to know is whether the measurements are per-thread or > per-process. Some estimates on the overhead when this is enabled and when > it > isn't would also be appreciated. > > https://chromiumcodereview.appspot.com/2163783003/ > -- 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.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nico; gentle nudge for owners please...
lgtm I apologize for the bikeshed comment, feel free to ignore. https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:208: CHECK_EQ(false, g_heap_tracking_enabled) << "No double-enabling."; check isn't thread-safe (doubt it matters much though) https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:70: static ThreadAllocatorUsage Now(); nit: "Now()" sounds a bit like time, maybe "CurrentUsage()"
https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:208: CHECK_EQ(false, g_heap_tracking_enabled) << "No double-enabling."; On 2016/09/09 14:40:33, Nico wrote: > check isn't thread-safe (doubt it matters much though) InsertAllocatorDispatch has a DCHECK(CalledOnValidThread()), which checks that all registrations are done on the same thread (in dcheck builds). So, even if you manage to race on the g_tracker_enabled flag here which is not atomic, this should hit anyway that dcheck.
Thanks, committing. https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:208: CHECK_EQ(false, g_heap_tracking_enabled) << "No double-enabling."; On 2016/09/09 14:40:33, Nico wrote: > check isn't thread-safe (doubt it matters much though) Yeah, this is best effort. https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.cc:208: CHECK_EQ(false, g_heap_tracking_enabled) << "No double-enabling."; On 2016/09/09 14:56:18, Primiano Tucci wrote: > On 2016/09/09 14:40:33, Nico wrote: > > check isn't thread-safe (doubt it matters much though) > > InsertAllocatorDispatch has a DCHECK(CalledOnValidThread()), which checks that > all registrations are done on the same thread (in dcheck builds). So, even if > you manage to race on the g_tracker_enabled flag here which is not atomic, this > should hit anyway that dcheck. Ah, nice. I figure this is a dev-time buzz only, so ought to be fine. https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... File base/debug/scoped_thread_heap_usage.h (right): https://codereview.chromium.org/2163783003/diff/700001/base/debug/scoped_thre... base/debug/scoped_thread_heap_usage.h:70: static ThreadAllocatorUsage Now(); On 2016/09/09 14:40:33, Nico wrote: > nit: "Now()" sounds a bit like time, maybe "CurrentUsage()" Thanks, that's a much better name. So amended.
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, chrisha@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2163783003/#ps720001 (title: "Address Nico's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. This uses the generic allocator shim to hook into heap allocations. When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation. Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled. BUG=644385 ========== to ========== Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. This uses the generic allocator shim to hook into heap allocations. When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation. Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled. BUG=644385 ==========
Message was sent while issue was closed.
Committed patchset #28 (id:720001)
Message was sent while issue was closed.
Description was changed from ========== Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. This uses the generic allocator shim to hook into heap allocations. When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation. Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled. BUG=644385 ========== to ========== Implement a ScopedThreadHeapUsage class to allow profiling per-thread heap usage. This uses the generic allocator shim to hook into heap allocations. When disabled and unused, there is no runtime penalty for this. When heap tracing is enabled, there's a small accounting overhead for every allocation. Instantiating a ScopedThreadHeapUsage instance carries O(1) cost, whether or not heap tracing is enabled. BUG=644385 Committed: https://crrev.com/46e1b077586d5b91f9abb7a6a8f9eb6558a584bd Cr-Commit-Position: refs/heads/master@{#417601} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/46e1b077586d5b91f9abb7a6a8f9eb6558a584bd Cr-Commit-Position: refs/heads/master@{#417601}
Message was sent while issue was closed.
On 2016/09/09 16:46:09, commit-bot: I haz the power wrote: > Patchset 28 (id:??) landed as > https://crrev.com/46e1b077586d5b91f9abb7a6a8f9eb6558a584bd > Cr-Commit-Position: refs/heads/master@{#417601} This is breaking the SyzyAsan builder (http://build.chromium.org/p/chromium.lkgr/builders/Win%20SyzyASAN%20LKGR/buil...): Writing """\ disable_precompiled_headers = true is_debug = false is_syzyasan = true target_cpu = "x86" """ to C:\b\c\b\Win_SyzyASAN_LKGR\src\out\Release\args.gn. C:\b\c\b\Win_SyzyASAN_LKGR\src\buildtools\win\gn.exe gen //out/Release --check -> returned 1 ERROR at //base/allocator/winheap_stubs_win.cc:16:11: Include not allowed. #include "base/bits.h" ^---------- It is not in any dependency of //base/allocator:allocator_shim The include file is in the target(s): //base:base which should somehow be reachable. GN gen failed: 1 step returned non-zero exit code: 1 @@@STEP_FAILURE@@@ |
