Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623613003:
Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… (Closed)
|
Created:
3 years, 11 months ago by bkonyi Modified:
3 years, 11 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmalloc. Also updated BUILD.gn files to account for include paths within the tcmalloc project.
BUG=
R=zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/bcd7ba105645d24813333c4ca5bb55b202a6a563
Patch Set 1 #Patch Set 2 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 3
Patch Set 3 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 36
Patch Set 4 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 28
Patch Set 5 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 12
Patch Set 6 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 2
Patch Set 7 : Changed setter for malloc hooks state from SetInitialized to set_initialized #
Total comments: 6
Patch Set 8 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #Patch Set 9 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 27
Patch Set 10 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #Patch Set 11 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 10
Patch Set 12 : Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmall… #
Total comments: 1
Messages
Total messages: 31 (3 generated)
bkonyi@google.com changed reviewers: + asiva@google.com, zra@google.com
This change introduces some simple tracking of native heap memory allocations using tcmalloc hooks. There were a few areas where I ran into macro naming conflicts from files included from tcmalloc, but they've been resolved. The comments are on patch 2 since patch 3 just contains a formatting fix. https://codereview.chromium.org/2623613003/diff/20001/runtime/platform/assert.h File runtime/platform/assert.h (right): https://codereview.chromium.org/2623613003/diff/20001/runtime/platform/assert... runtime/platform/assert.h:303: CompileAssert is also defined in tcmalloc. Since it's only used here, I placed it into the dart namespace to avoid conflicts. https://codereview.chromium.org/2623613003/diff/20001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/20001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:14: #pragma push_macro("COMPILE_ASSERT") These macros conflict with macros of the same name defined in files included from the tcmalloc project. I'm not sure if there's a preferred way of dealing with macro conflicts, but this seems to work. https://codereview.chromium.org/2623613003/diff/20001/runtime/vm/malloc_hooks... File runtime/vm/malloc_hooks_test.cc (right): https://codereview.chromium.org/2623613003/diff/20001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:72: } The next patch set fixes this formatting mistake.
johnmccutchan@google.com changed reviewers: + johnmccutchan@google.com
DBC and I'm wondering why this code is inside the VM and not inside the embedder? https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:47: ASSERT(MallocHook::AddNewHook(&RecordAllocHook)); Code inside ASSERT blocks does not run in release mode. If this is intentional, maybe add a comment above to indicate. Also you probably don't want to allocate the memory in release mode either. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:56: initialized_ = false; do you need to free malloc_hooks_memory_ ? Dart_Initialize/Dart_Cleanup could be called more than once.
https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:47: ASSERT(MallocHook::AddNewHook(&RecordAllocHook)); On 2017/01/10 at 23:37:54, Cutch wrote: > Code inside ASSERT blocks does not run in release mode. If this is intentional, maybe add a comment above to indicate. > > Also you probably don't want to allocate the memory in release mode either. Good point. I'm fairly certain it wouldn't make much sense to have these hooks unless we're in debug mode right? https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:56: initialized_ = false; On 2017/01/10 at 23:37:54, Cutch wrote: > do you need to free malloc_hooks_memory_ ? Dart_Initialize/Dart_Cleanup could be called more than once. I didn't realize Dart_Initialize/Dart_Cleaup can be called multiple times. I'll go ahead and free the malloc_hooks_memory_ and anything else allocated in MallocHooks::Init().
On 2017/01/10 at 23:37:54, johnmccutchan wrote: > DBC and I'm wondering why this code is inside the VM and not inside the embedder? > > https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.cc > File runtime/vm/malloc_hooks.cc (right): > > https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... > runtime/vm/malloc_hooks.cc:47: ASSERT(MallocHook::AddNewHook(&RecordAllocHook)); > Code inside ASSERT blocks does not run in release mode. If this is intentional, maybe add a comment above to indicate. > > Also you probably don't want to allocate the memory in release mode either. > > https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... > runtime/vm/malloc_hooks.cc:56: initialized_ = false; > do you need to free malloc_hooks_memory_ ? Dart_Initialize/Dart_Cleanup could be called more than once. You should probably discuss this with @zra, as he suggested that we should keep this code inside of the VM.
initial comments https://codereview.chromium.org/2623613003/diff/40001/runtime/BUILD.gn File runtime/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/BUILD.gn#newcod... runtime/BUILD.gn:151: include_dirs = [ "." ] An empty list should work here, too. https://codereview.chromium.org/2623613003/diff/40001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:303: defines += [ "DART_USE_TCMALLOC" ] gen_snapshot already includes dart_config, so I think all that should be needed here is the additional deps on //third_party/tcmalloc. https://codereview.chromium.org/2623613003/diff/40001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:585: defines += [ "DART_USE_TCMALLOC" ] ditto https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... File runtime/vm/dart_api_impl.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1164: MallocHooks::Init(); It looks like we've been tending to follow the convention of putting initializer calls like this instead in Dart::InitOnce() called below (it's in runtime/vm/dart.cc). We'll only miss these two allocations that only happen when we'll be bailing out for an error anyway, so I think we can follow suit. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1177: Extra newline here. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1188: MallocHooks::TearDown(); As with the Init call, let's move this to the *bottom* of Dart::Cleanup(). https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:21: #include "base/low_level_alloc.h" These includes should go in malloc_hooks.cc https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:44: #if defined(DART_USE_TCMALLOC) These calls and state should go in a class in malloc_hooks.cc. Avoiding having "DART_USE_TCMALLOC" in this file would be the goal.
https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... File runtime/vm/dart_api_impl.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1164: MallocHooks::Init(); On 2017/01/11 at 00:07:31, zra wrote: > It looks like we've been tending to follow the convention of putting initializer calls like this instead in Dart::InitOnce() called below (it's in runtime/vm/dart.cc). We'll only miss these two allocations that only happen when we'll be bailing out for an error anyway, so I think we can follow suit. Placing the MallocHooks::Init() call in Dart::InitOnce() actually causes issues with the current implementation and the tests since Dart::InitOnce() is called when run_vm_tests is executed, resulting in all the allocations done in the tests being recorded. I could add MallocHooks::Reset() to create a clean state for the tests if we're dead set on placing MallocHooks::Init() in Dart::InitOnce().
On 2017/01/11 00:58:23, bkonyi wrote: > https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... > File runtime/vm/dart_api_impl.cc (right): > > https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... > runtime/vm/dart_api_impl.cc:1164: MallocHooks::Init(); > On 2017/01/11 at 00:07:31, zra wrote: > > It looks like we've been tending to follow the convention of putting > initializer calls like this instead in Dart::InitOnce() called below (it's in > runtime/vm/dart.cc). We'll only miss these two allocations that only happen when > we'll be bailing out for an error anyway, so I think we can follow suit. > > Placing the MallocHooks::Init() call in Dart::InitOnce() actually causes issues > with the current implementation and the tests since Dart::InitOnce() is called > when run_vm_tests is executed, resulting in all the allocations done in the > tests being recorded. I could add MallocHooks::Reset() to create a clean state > for the tests if we're dead set on placing MallocHooks::Init() in > Dart::InitOnce(). Yes, I think that's the right approach.
After seeing what you have to do to use tcmalloc's data structures/functions, I think we should find another way to do what we need. Grab me when you get in tomorrow morning and will sit together and hammer things out. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:18: static SpinLock malloc_hooks_spinlock_(base::LINKER_INITIALIZED); We should use our own Mutex and MutexLocker from vm/os_thread.h and vm/lockers.h. The mutex can be a static member of the class holding the state in this file. That way we can avoid dependence on tcmalloc internals in its base/. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:47: ASSERT(MallocHook::AddNewHook(&RecordAllocHook)); On 2017/01/10 23:43:40, bkonyi wrote: > On 2017/01/10 at 23:37:54, Cutch wrote: > > Code inside ASSERT blocks does not run in release mode. If this is > intentional, maybe add a comment above to indicate. > > > > Also you probably don't want to allocate the memory in release mode either. > > Good point. I'm fairly certain it wouldn't make much sense to have these hooks > unless we're in debug mode right? Let's leave it in Release mode, and do some Golem runs when it's finished to decide if it should be Debug-only. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:21: #include "base/low_level_alloc.h" On 2017/01/11 00:07:31, zra wrote: > These includes should go in malloc_hooks.cc I've been thinking more about why this is a bit messy. It's because we're trying to use tcmalloc-internal data structures/functions, when doing so is really violating an API boundary. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:41: static uintptr_t allocation_count_; These two are also not needed here in the generic interface as allocation_count() and heap_allocated_memory_in_bytes() both return 0 in the _unsupported case. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... File runtime/vm/malloc_hooks_test.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:16: void MallocHookTestBufferInitializer(volatile char* buffer, uintptr_t size) { static https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:30: volatile char* buffer = new volatile char[buffer_size]; I don't think volatile is needed here. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:46: volatile char* pre_hook_buffer = new volatile char[pre_hook_buffer_size]; ditto https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/vm_sources.gypi File runtime/vm/vm_sources.gypi (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/vm_sources.g... runtime/vm/vm_sources.gypi:289: 'malloc_hooks_unsupported.cc', alphabetize
I've finished re-implementing the malloc_hooks code to not be dependent on internal data structures of tcmalloc. There is a lot changed in this new CL, so I'm sure there will be lots of comments. https://codereview.chromium.org/2623613003/diff/40001/runtime/BUILD.gn File runtime/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/BUILD.gn#newcod... runtime/BUILD.gn:151: include_dirs = [ "." ] On 2017/01/11 at 00:07:31, zra wrote: > An empty list should work here, too. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:303: defines += [ "DART_USE_TCMALLOC" ] On 2017/01/11 at 00:07:31, zra wrote: > gen_snapshot already includes dart_config, so I think all that should be needed here is the additional deps on //third_party/tcmalloc. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... File runtime/vm/dart_api_impl.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1164: MallocHooks::Init(); On 2017/01/11 at 00:58:23, bkonyi wrote: > On 2017/01/11 at 00:07:31, zra wrote: > > It looks like we've been tending to follow the convention of putting initializer calls like this instead in Dart::InitOnce() called below (it's in runtime/vm/dart.cc). We'll only miss these two allocations that only happen when we'll be bailing out for an error anyway, so I think we can follow suit. > > Placing the MallocHooks::Init() call in Dart::InitOnce() actually causes issues with the current implementation and the tests since Dart::InitOnce() is called when run_vm_tests is executed, resulting in all the allocations done in the tests being recorded. I could add MallocHooks::Reset() to create a clean state for the tests if we're dead set on placing MallocHooks::Init() in Dart::InitOnce(). Added Reset() method. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1177: On 2017/01/11 at 00:07:31, zra wrote: > Extra newline here. Removed. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/dart_api_imp... runtime/vm/dart_api_impl.cc:1188: MallocHooks::TearDown(); On 2017/01/11 at 00:07:31, zra wrote: > As with the Init call, let's move this to the *bottom* of Dart::Cleanup(). Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:18: static SpinLock malloc_hooks_spinlock_(base::LINKER_INITIALIZED); On 2017/01/11 at 07:12:45, zra wrote: > We should use our own Mutex and MutexLocker from vm/os_thread.h and vm/lockers.h. The mutex can be a static member of the class holding the state in this file. That way we can avoid dependence on tcmalloc internals in its base/. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:47: ASSERT(MallocHook::AddNewHook(&RecordAllocHook)); On 2017/01/10 at 23:37:54, Cutch wrote: > Code inside ASSERT blocks does not run in release mode. If this is intentional, maybe add a comment above to indicate. > > Also you probably don't want to allocate the memory in release mode either. I've addressed the code in ASSERT blocks not running in release mode. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:56: initialized_ = false; On 2017/01/10 at 23:43:40, bkonyi wrote: > On 2017/01/10 at 23:37:54, Cutch wrote: > > do you need to free malloc_hooks_memory_ ? Dart_Initialize/Dart_Cleanup could be called more than once. > > I didn't realize Dart_Initialize/Dart_Cleaup can be called multiple times. I'll go ahead and free the malloc_hooks_memory_ and anything else allocated in MallocHooks::Init(). Given the new implementation, this isn't applicable anymore. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:21: #include "base/low_level_alloc.h" On 2017/01/11 at 07:12:45, zra wrote: > On 2017/01/11 00:07:31, zra wrote: > > These includes should go in malloc_hooks.cc > > I've been thinking more about why this is a bit messy. It's because we're trying to use tcmalloc-internal data structures/functions, when doing so is really violating an API boundary. The new patch takes a new approach that doesn't violate API boundaries. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:41: static uintptr_t allocation_count_; On 2017/01/11 at 07:12:45, zra wrote: > These two are also not needed here in the generic interface as allocation_count() and heap_allocated_memory_in_bytes() both return 0 in the _unsupported case. Removed. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:44: #if defined(DART_USE_TCMALLOC) On 2017/01/11 at 00:07:31, zra wrote: > These calls and state should go in a class in malloc_hooks.cc. Avoiding having "DART_USE_TCMALLOC" in this file would be the goal. Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... File runtime/vm/malloc_hooks_test.cc (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:16: void MallocHookTestBufferInitializer(volatile char* buffer, uintptr_t size) { On 2017/01/11 at 07:12:45, zra wrote: > static Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:30: volatile char* buffer = new volatile char[buffer_size]; On 2017/01/11 at 07:12:45, zra wrote: > I don't think volatile is needed here. You're right. For some reason it seemed to make things work before, but it doesn't seem to matter now. Removed. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks_test.cc:46: volatile char* pre_hook_buffer = new volatile char[pre_hook_buffer_size]; On 2017/01/11 at 07:12:45, zra wrote: > ditto Done. https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/vm_sources.gypi File runtime/vm/vm_sources.gypi (right): https://codereview.chromium.org/2623613003/diff/40001/runtime/vm/vm_sources.g... runtime/vm/vm_sources.gypi:289: 'malloc_hooks_unsupported.cc', On 2017/01/11 at 07:12:45, zra wrote: > alphabetize Done?
https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:298: defines = [] Looks like this can be removed. https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:303: include_dirs += [ "../third_party/tcmalloc/gperftools/src" ] Still need this include dir here? It should also be coming in through dart_config. https://codereview.chromium.org/2623613003/diff/60001/runtime/platform/assert.h File runtime/platform/assert.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/platform/assert... runtime/platform/assert.h:302: namespace dart { Is this still needed? https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/dart.cc File runtime/vm/dart.cc (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/dart.cc#newc... runtime/vm/dart.cc:158: MallocHooks::Init(); Can this go higher up? I think it's not strictly necessary, but it'd be nice if we could capture allocations for as much of the initialization as possible. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h File runtime/vm/hash_map.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:313: void BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Remove( return bool. true on successful removal. false if the key isn't found. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:329: count_--; Maybe 'return true' after this decrement, and then you don't need the 'else' part. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:341: return; return true https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:353: return; return false https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:362: } return true https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:41: }; DISALLOW_ALLOCATION(); DISALLOW_COPY_AND_ASSIGN(MallocHookFlagHolder); https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:87: }; private: DISALLOW_COPY_AND_ASSIGN(AddressMap); https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:89: // MallocHooks state / locks. I think it would be better to wrap up this state in an all-static class instead of global variables. It would also hold the functions from the MallocHooks class in the header file that don't need to be called from other code---probably the private: functions there. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:112: success = MallocHook::AddNewHook(&RecordAllocHook); Is it the same hook for both malloc and new? https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:23: static void RecordAllocHook(const void* ptr, size_t size); These should go in a class in malloc_hooks.cc. This class should just be the interface that we call from the rest of the VM.
https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/dart.cc File runtime/vm/dart.cc (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/dart.cc#newc... runtime/vm/dart.cc:158: MallocHooks::Init(); On 2017/01/13 at 19:14:17, zra wrote: > Can this go higher up? I think it's not strictly necessary, but it'd be nice if we could capture allocations for as much of the initialization as possible. No, it can't since MutexLocker needs some things to be initialized before it can be used. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:112: success = MallocHook::AddNewHook(&RecordAllocHook); On 2017/01/13 at 19:14:18, zra wrote: > Is it the same hook for both malloc and new? Yes it is.
https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:298: defines = [] On 2017/01/13 at 19:14:17, zra wrote: > Looks like this can be removed. Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:303: include_dirs += [ "../third_party/tcmalloc/gperftools/src" ] On 2017/01/13 at 19:14:17, zra wrote: > Still need this include dir here? It should also be coming in through dart_config. Removed. https://codereview.chromium.org/2623613003/diff/60001/runtime/platform/assert.h File runtime/platform/assert.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/platform/assert... runtime/platform/assert.h:302: namespace dart { On 2017/01/13 at 19:14:17, zra wrote: > Is this still needed? Nope, removed. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h File runtime/vm/hash_map.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:313: void BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Remove( On 2017/01/13 at 19:14:17, zra wrote: > return bool. true on successful removal. false if the key isn't found. Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:329: count_--; On 2017/01/13 at 19:14:17, zra wrote: > Maybe 'return true' after this decrement, and then you don't need the 'else' part. Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:341: return; On 2017/01/13 at 19:14:17, zra wrote: > return true Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:353: return; On 2017/01/13 at 19:14:17, zra wrote: > return false Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/hash_map.h#n... runtime/vm/hash_map.h:362: } On 2017/01/13 at 19:14:17, zra wrote: > return true Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:41: }; On 2017/01/13 at 19:14:18, zra wrote: > DISALLOW_ALLOCATION(); > DISALLOW_COPY_AND_ASSIGN(MallocHookFlagHolder); Added. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:87: }; On 2017/01/13 at 19:14:17, zra wrote: > private: > DISALLOW_COPY_AND_ASSIGN(AddressMap); As discussed offline, can't do this here. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:89: // MallocHooks state / locks. On 2017/01/13 at 19:14:17, zra wrote: > I think it would be better to wrap up this state in an all-static class instead of global variables. It would also hold the functions from the MallocHooks class in the header file that don't need to be called from other code---probably the private: functions there. Done. https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/60001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:23: static void RecordAllocHook(const void* ptr, size_t size); On 2017/01/13 at 19:14:18, zra wrote: > These should go in a class in malloc_hooks.cc. This class should just be the interface that we call from the rest of the VM. Done.
https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/dart.cc File runtime/vm/dart.cc (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/dart.cc#newc... runtime/vm/dart.cc:158: MallocHooks::Init(); Is it possible to move this up? Maybe before OS::InitOnce()? https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:68: template <typename K, typename V> Are these template arguments used? It looks like we don't need to template this class. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:97: static bool initialized; What we typically do is make fields like this private, and add getters (and where needed, setters) for them. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:183: if (!MallocHooksState::initialized || I think we probably want to ASSERT(MallocHookState::initialized); We also need to hold the lock consistently when accessing MallocHookState::initialized https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:202: if (!MallocHooksState::initialized || ditto https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:22: // Hooks. Probably don't need this comment anymore either.
https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/dart.cc File runtime/vm/dart.cc (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/dart.cc#newc... runtime/vm/dart.cc:158: MallocHooks::Init(); On 2017/01/13 at 23:30:37, zra wrote: > Is it possible to move this up? Maybe before OS::InitOnce()? Nope, this is the absolute earliest we can call this without hitting asserts. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks.cc File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:68: template <typename K, typename V> On 2017/01/13 at 23:30:37, zra wrote: > Are these template arguments used? It looks like we don't need to template this class. You're right. They were used before, but I must have forgotten to remove this. Removed. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:97: static bool initialized; On 2017/01/13 at 23:30:37, zra wrote: > What we typically do is make fields like this private, and add getters (and where needed, setters) for them. Done. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:183: if (!MallocHooksState::initialized || On 2017/01/13 at 23:30:37, zra wrote: > I think we probably want to ASSERT(MallocHookState::initialized); > > We also need to hold the lock consistently when accessing MallocHookState::initialized Done. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.cc:202: if (!MallocHooksState::initialized || On 2017/01/13 at 23:30:37, zra wrote: > ditto Done. https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/80001/runtime/vm/malloc_hooks... runtime/vm/malloc_hooks.h:22: // Hooks. On 2017/01/13 at 23:30:37, zra wrote: > Probably don't need this comment anymore either. Removed.
Let's discuss how to get performance numbers on Tuesday morning. https://codereview.chromium.org/2623613003/diff/100001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/100001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:97: static void SetInitialized() { initialized_ = true; } It's our usual style to call setters like this set_initialized
https://codereview.chromium.org/2623613003/diff/100001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/100001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:97: static void SetInitialized() { initialized_ = true; } On 2017/01/17 at 03:35:06, zra wrote: > It's our usual style to call setters like this set_initialized Done.
https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:5: #if defined(DART_USE_TCMALLOC) #include "platform/globals.h" #if defined(DART_USE_TCMALLOC) && !defined(PRODUCT) https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks_test.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks_test.cc:5: #if defined(DART_USE_TCMALLOC) ditto https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks_unsupported.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks_unsupported.cc:5: #if !defined(DART_USE_TCMALLOC) #include "platform/globals.h" #if !defined(DART_USE_TCMALLOC) || defined(PRODUCT)
https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:6: Done. https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks_test.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks_test.cc:5: #if defined(DART_USE_TCMALLOC) On 2017/01/17 at 22:30:50, zra wrote: > ditto Done. https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks_unsupported.cc (right): https://codereview.chromium.org/2623613003/diff/120001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks_unsupported.cc:5: #if !defined(DART_USE_TCMALLOC) On 2017/01/17 at 22:30:50, zra wrote: > #include "platform/globals.h" > #if !defined(DART_USE_TCMALLOC) || defined(PRODUCT) Done.
DBC https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:38: return OSThread::GetThreadLocal(in_malloc_hook_flag_); ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey); https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:49: // RawPointerKeyValueTrait, the default value is -1 as 0 can be a valid entry. When would an allocation have a size of 0? https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:68: two blank lines (here and elsewhere) https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:111: static void IncrementHeapAllocatedMemoryInBytes(uintptr_t size) { Could this and Increment/DecrementAllocationCount be merged? https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:192: void MallocHooks::Reset() { This should probably be named ResetStats because it's just a wrapper around a call to ResetStats. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:214: void MallocHooksState::RecordAllocHook(const void* ptr, size_t size) { Could this cause us to lose allocations that occur as a side effect of an allocation? We definitely avoid counting memory associated with the memory map but what about other cases?
https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:135: static Mutex malloc_hook_mutex_; Just ran into this while running the code on DebugIA32. There is a problem in apps that call Platform.exit(). We call the low-level C exit() without doing Dart_Cleanup(). The low-level C exit() runs the destructor for this mutex. It is permitted to call the destructor whenever it wants, so it may be called before subsequent calls to malloc()/free(). The solution to this problem that we take in the VM is to allocate the lock on the heap in the Init() call, and then never deallocate it.
https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:21: class MallocHookFlagHolder { Maybe MallocHookScope? MallocHookSingleEntryScope if we want to be extra verbose. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:37: static bool IsMallocHookFlagHeld() { Maybe IsInHook() https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:99: static void set_initialized() { initialized_ = true; } set_initialized(bool b) so we can set it to false https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:156: void MallocHooks::Init() { I don't think we can support this call potentially being made from multiple threads. You can add a comment to that effect, or just call it InitOnce() like we do with other similar calls. Then, you can allocate the lock first, then take the lock, then ASSERT that we're not initialized, and then initializing all the things. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:176: if (!MallocHooksState::initialized()) { Can we ASSERT that we're initialized here instead? https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:179: Set initialized to false.
https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:21: class MallocHookFlagHolder { On 2017/01/18 at 18:26:12, zra wrote: > Maybe MallocHookScope? MallocHookSingleEntryScope if we want to be extra verbose. Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:37: static bool IsMallocHookFlagHeld() { On 2017/01/18 at 18:26:12, zra wrote: > Maybe IsInHook() Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:38: return OSThread::GetThreadLocal(in_malloc_hook_flag_); On 2017/01/18 at 17:14:00, Cutch wrote: > ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey); Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:49: // RawPointerKeyValueTrait, the default value is -1 as 0 can be a valid entry. On 2017/01/18 at 17:14:00, Cutch wrote: > When would an allocation have a size of 0? There's a few places it happens, but this is an example: https://github.com/dart-lang/sdk/blob/master/runtime/vm/dart_api_message.cc#L.... https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:68: On 2017/01/18 at 17:14:00, Cutch wrote: > two blank lines (here and elsewhere) Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:99: static void set_initialized() { initialized_ = true; } On 2017/01/18 at 18:26:12, zra wrote: > set_initialized(bool b) so we can set it to false The only time initialized_ is set to false is after we tear down the hooks. MallocHooks::TearDown() calls MallocHooksState::Reset() which resets all the state, including the initialized flag. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:111: static void IncrementHeapAllocatedMemoryInBytes(uintptr_t size) { On 2017/01/18 at 17:14:00, Cutch wrote: > Could this and Increment/DecrementAllocationCount be merged? Do you mean the Increment/DecrementAllocationCount on lines 104/105? https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:135: static Mutex malloc_hook_mutex_; On 2017/01/18 at 17:30:44, zra wrote: > Just ran into this while running the code on DebugIA32. There is a problem in apps that call Platform.exit(). We call the low-level C exit() without doing Dart_Cleanup(). The low-level C exit() runs the destructor for this mutex. It is permitted to call the destructor whenever it wants, so it may be called before subsequent calls to malloc()/free(). > > The solution to this problem that we take in the VM is to allocate the lock on the heap in the Init() call, and then never deallocate it. Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:156: void MallocHooks::Init() { On 2017/01/18 at 18:26:12, zra wrote: > I don't think we can support this call potentially being made from multiple threads. You can add a comment to that effect, or just call it InitOnce() like we do with other similar calls. Then, you can allocate the lock first, then take the lock, then ASSERT that we're not initialized, and then initializing all the things. I've gone ahead and changed it to InitOnce(). https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:176: if (!MallocHooksState::initialized()) { On 2017/01/18 at 18:26:12, zra wrote: > Can we ASSERT that we're initialized here instead? Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:179: On 2017/01/18 at 18:26:12, zra wrote: > Set initialized to false. MallocHooksState::Reset() does this. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:192: void MallocHooks::Reset() { On 2017/01/18 at 17:14:00, Cutch wrote: > This should probably be named ResetStats because it's just a wrapper around a call to ResetStats. Done. https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:214: void MallocHooksState::RecordAllocHook(const void* ptr, size_t size) { On 2017/01/18 at 17:14:00, Cutch wrote: > Could this cause us to lose allocations that occur as a side effect of an allocation? We definitely avoid counting memory associated with the memory map but what about other cases? It shouldn't since operator new returns before the constructor is called, so the hooks exit before any other allocations are done by the constructor.
https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/160001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:111: static void IncrementHeapAllocatedMemoryInBytes(uintptr_t size) { On 2017/01/18 at 20:02:42, bkonyi wrote: > On 2017/01/18 at 17:14:00, Cutch wrote: > > Could this and Increment/DecrementAllocationCount be merged? > > Do you mean the Increment/DecrementAllocationCount on lines 104/105? Done.
https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:35: MallocHookScope() { OSThread::SetThreadLocal(in_malloc_hook_flag_, 1); } Let's add ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey); here too. https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:37: ~MallocHookScope() { OSThread::SetThreadLocal(in_malloc_hook_flag_, 0); } ditto https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:56: typedef void* Key; Can this be const void* to avoid the const_casts below? https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:123: --allocation_count_; ASSERT(allocation_count_ > 0); https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.h:18: static uintptr_t allocation_count(); If the type we're tracking in the hashmap is intptr_t, let's just make it intptr_t top-to-bottom.
https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks.cc (right): https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:35: MallocHookScope() { OSThread::SetThreadLocal(in_malloc_hook_flag_, 1); } On 2017/01/18 at 21:53:30, zra wrote: > Let's add ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey); here too. Done. https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:37: ~MallocHookScope() { OSThread::SetThreadLocal(in_malloc_hook_flag_, 0); } On 2017/01/18 at 21:53:30, zra wrote: > ditto Done. https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:56: typedef void* Key; On 2017/01/18 at 21:53:30, zra wrote: > Can this be const void* to avoid the const_casts below? Done. https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.cc:123: --allocation_count_; On 2017/01/18 at 21:53:30, zra wrote: > ASSERT(allocation_count_ > 0); Done. https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hooks.h File runtime/vm/malloc_hooks.h (right): https://codereview.chromium.org/2623613003/diff/200001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks.h:18: static uintptr_t allocation_count(); On 2017/01/18 at 21:53:30, zra wrote: > If the type we're tracking in the hashmap is intptr_t, let's just make it intptr_t top-to-bottom. Done.
lgtm
Description was changed from ========== Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmalloc. Also updated BUILD.gn files to account for include paths within the tcmalloc project. BUG= ========== to ========== Implemented basic heap memory allocation tracking in MallocHooks using hooks registered with tcmalloc. Also updated BUILD.gn files to account for include paths within the tcmalloc project. BUG= R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/bcd7ba105645d24813333c4ca5bb55b202a6a563 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) manually as bcd7ba105645d24813333c4ca5bb55b202a6a563 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2623613003/diff/220001/runtime/vm/malloc_hook... File runtime/vm/malloc_hooks_unsupported.cc (right): https://codereview.chromium.org/2623613003/diff/220001/runtime/vm/malloc_hook... runtime/vm/malloc_hooks_unsupported.cc:13: void MallocHooks::Init() { Looks like these definitions didn't get updated after some other changes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
