Port allocator shim to Mac OSX |
|||||||||||||||
Issue descriptionmacOS: 10.11.6 Chrome: Canary or ToT Start Chrome with --enable-heap-profiling. Take a memory-infra trace. Click on a memory icon. Note that many of the columns have a triple-hotdog view showing more information, but the malloc column does not.
,
Nov 15 2016
+dskiba I think he tried something recently
,
Nov 15 2016
Here is my mail from back then: " Hi, I've hacked together an allocator shim for macOS to be able to do work on my MacBook. CL is here: https://codereview.chromium.org/2499373003 It's based on base/process/memory_mac.mm. I also fixed tests to pass. Things learned: 1. AllocatorDispatch::GetSizeEstimateFn() should take 'const void*', not 'void*'. I had to do nasty const_cast<> in ShimGetSizeEstimate(). Both HeapSize (on Windows) and malloc_usable_size() take 'const void*'. 2. macOS realloc() is not a simple stub to zone->realloc(). It looks up malloc zone for 'ptr' argument (and aborts if that fails, so trick with realloc(0x420) from tests doesn't work.). And if 'ptr' argument is nullptr, it simply calls zone->malloc(). I had to fix / disable tests. 3. Some functions in malloc zone depend on version. For example for version >= 6 zone->free_definite_size() is called instead of zone->free(). So I had to hook it too. 4. attribute((alias("fn"))) is not supported on macOS. attribute((weakref("fn"))) is supported, but only if the function has internal linkage. So I had to reimplement new/delete operators and just call appropriate ShimCppXXX functions. 5. There is a difference between ShimCppNew and ShimMalloc, but it's not tested by InterceptCppSymbols test. 6. Also updated AllocatorShimTest to hook get_size_estimation. TODO: 1. base/process/memory_mac.mm hooks more stuff - purgeable_zone, CFAllocators and even [NSObject allocWithZone:]. Allocator shim probably should too. 2. Whole process of hooking memory zones must be triggered at runtime, it can't happen statically. Right now this is done in a ctor of a global object. Maybe there is better way (__attribute__((constructor(0)))?). 3. Fix InterceptCppSymbols test. 4. Fix AllocatorDispatch::GetSizeEstimateFn() to take 'const void*' (might do this myself). "
,
Nov 16 2016
@alph, might this be related to: 665536 ?
,
Nov 16 2016
This is not a devtools issue. I know that there is a proiler also there, but this different and is relative to https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/heap_profiler.md Issue 665536 is orthogonal, the reason is simply code missing as explained in #1. Happy if somebody wants to champion this area but let's not overload our devtools friends with bugs that are not their :)
,
Nov 25 2016
,
Nov 29 2016
I did some experiments, will implement this feature.
,
Nov 29 2016
Awesome! Thank you :)
,
Dec 14 2016
It looks like we have a brave volunteer to take this over. Thanks Erik!
,
Dec 19 2016
There are several potential mechanisms for overriding malloc. tcmalloc's documentation describes why the approach [malloc zone modification] currently used in Chrome, and in dskiba's CL is better than the alternatives: https://cs.chromium.org/chromium/src/third_party/tcmalloc/chromium/src/libc_override_osx.h?l=37 The main downside is that this misses out on allocations before malloc zone modification. tcmalloc uses a different mechanism for modifying the malloc zone. https://cs.chromium.org/chromium/src/third_party/tcmalloc/chromium/src/libc_override_osx.h?l=271 This has the benefit of correctly handling allocations made before the zone change. Also, it does not require calling mach_vm_region/mach_vm_protect.
,
Dec 20 2016
As an aside, jemalloc does the same thing as tcmalloc: https://github.com/jemalloc/jemalloc/blob/dev/src/zone.c But also registers/unregisters the purgeable zone.
,
Dec 20 2016
Reading the source for jemalloc shows that 10.12 also has a "lite zone" when malloc stack logging is enabled. Upstream tcmalloc has the same code/comment: https://github.com/gperftools/gperftools/blob/master/src/libc_override_osx.h
,
Dec 20 2016
Re #10: my main concern there is stability. These allocator change can have very subtle effects that are very hard to predict, see historical comments in [1]. If I was doing this, I'd very paranoidly just adapt what we have today which is guaranteed to work. At the same time, I don't know absolutely anything about OSX, you folks are the experts here so I won't rule out (nor I'm going to oppose to) other, better, approaches. Just my tip here would be make sure that OSX base owners (mark@ and rsesek@ might be good candidates) are fine with that. Also note that we never used tcmalloc (the one in third_party) on OSX, we always used the system one. The only cases in which we use(d) the checked in version of tcmalloc are: - Linux: since forever and still today. - Android: only in custom profiling builds for a while to make DMP (Deep Memory Profiler, now nuked from orbit) work. Was quite crashy. - Windows: there was a time where tcmalloc was used there. But then we switched to WinHeap a while ago. - OSX: never So all the OSX code that you see there has never been hit in chrome and, as far as we know, might not work. [1] https://cs.chromium.org/chromium/src/third_party/tcmalloc/vendor/src/libc_override_glibc.h?rcl=0&l=66
,
Dec 22 2016
+mark [macOS guru] All heap-based memory allocations will eventually go through the mach vm subsystem. On macOS, malloc [libsystem_malloc.dylib, bsd/kern/kern_malloc.c] uses an additional abstraction layer of zones. Most frameworks will use the default zone [or perhaps the default purgeable zone], but frameworks are also allowed to register their own zones [and presumably use a custom implementation based directly on the mach vm system calls]. When I launch Chrome through lldb and log calls to malloc_zone_register and malloc_create_zone in the browser process, I see three new zones registered from libGLImage [malloc_create_zone], QuartzCore [malloc_zone_register] and ImageIO [malloc_zone_register]. When I print information about all zones in the process, I see 5: * default zone [~32MB] * purgeable zone, libGLImage, QuartzCore, ImageIO [no allocations] Presumably the default zone and purgeable zone were inherited from the parent process. My examination of the implementation of CFAllocator [CoreFoundation, see _CFAllocatorAllocate] and NSZone [libobjc, see _class_createInstanceFromZone] show that they call through to malloc zones. Overriding all malloc zones will allow us to intercept non-shared-memory heap allocations. There is no way [short of interposition] of determining when new zones are registered. Luckily, most memory appears to be allocated through the default zone. Given that malloc zones expose their total memory usage, I suggest that we only intercept default malloc zone allocations for the allocator shim. We will still know how much memory we're failing to track in the allocator shim.
,
Dec 22 2016
In the future, if we deem it necessary, we could theoretically track zones independently [and even have pseudo-zones for CoreFoundation and ObjC objects]. The main problem is that we would have to refactor the current implementation of the allocator shim [https://cs.chromium.org/chromium/src/base/allocator/README.md]. This seems like premature optimization to me - we can do it we think it's necessary. In the short term, we should: 1) Get the patch from c#3 working. 1a) We need to make sure batch_malloc and batch_free are intercepted as they get used by CF and ObjC, which the patch does not do. 1b) Fix the other issues raised in c#3. In the medium term, we should: 2) Figure out the right way to intercept malloc zone calls. Note that jemalloc and tcmalloc [see c#10, c#11] accomplish this without needing to change the protection on pages. This seems cleaner. mark@, perhaps you can comment?
,
Dec 22 2016
whoops, and the source link for malloc should be [libsystem_malloc.dylib, libmalloc-116/src/malloc.c]
,
Jan 3 2017
Responding #c15: I don’t know that what jemalloc and tcmalloc do is any “cleaner.” Certainly as far as the custom allocator zone itself is concerned, it’s cleaner, but the “promotion” of the custom zone to “default” is tricky and fragile. That’s not to say that the alternatives aren’t tricky or fragile, and certainly we’d want test coverage to ensure that we retain our desired behavior over time (and with OS updates) in any case. I’m just pointing out that each approach has its quirks. Anyway, I’m not sure that I’d call out the protection stuff as a limitation per se. When you register your own zone, libmalloc’s going to need to twiddle the protection bits momentarily too. Sure, there’s a difference in that it handles it for you, and it handles it under its own lock so there’s less room for racy badness, so that’s nice. I’m only mentioning it because you mentioned protection a couple of times, but that doesn’t really stand out as a prime concern to me. Like I said, the nicest thing about what jemalloc and tcmalloc do is that they use the zone feature in a more natural way, so that service for preexisting allocations doesn’t wind up going to a zone that’s never heard of the memory. On the other hand, with a good enough way to route preexisting allocations to their original zone, it’s awfully nice in some cases to be able to take over allocation requests to non-default zones too…
,
Jan 4 2017
,
Jan 4 2017
,
Jan 11 2017
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9550b71347ce9acbe2875fb75ea46ec096230506 commit 9550b71347ce9acbe2875fb75ea46ec096230506 Author: erikchen <erikchen@chromium.org> Date: Thu Jan 26 05:36:58 2017 Move logic from memory_mac to allocator_interception_mac. This CL is a refactor and has no intended behavior changes. This CL moves the allocator interception logic from base/process/memory_mac.mm to base/allocator/allocator_interception_mac.mm in preparation for an implementation of the allocator shim. BUG= 665567 Review-Url: https://codereview.chromium.org/2649973003 Cr-Commit-Position: refs/heads/master@{#446234} [modify] https://crrev.com/9550b71347ce9acbe2875fb75ea46ec096230506/base/BUILD.gn [add] https://crrev.com/9550b71347ce9acbe2875fb75ea46ec096230506/base/allocator/allocator_interception_mac.h [add] https://crrev.com/9550b71347ce9acbe2875fb75ea46ec096230506/base/allocator/allocator_interception_mac.mm [modify] https://crrev.com/9550b71347ce9acbe2875fb75ea46ec096230506/base/process/memory_mac.mm
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50a11072abe7c112ed23956929b3ac4d0590f6ff commit 50a11072abe7c112ed23956929b3ac4d0590f6ff Author: erikchen <erikchen@chromium.org> Date: Thu Jan 26 06:35:43 2017 Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions. This CL has no intended behavior change. Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim to easily integrate with the existing malloc interception code. BUG= 665567 Review-Url: https://codereview.chromium.org/2650363002 Cr-Commit-Position: refs/heads/master@{#446255} [modify] https://crrev.com/50a11072abe7c112ed23956929b3ac4d0590f6ff/base/allocator/allocator_interception_mac.h [modify] https://crrev.com/50a11072abe7c112ed23956929b3ac4d0590f6ff/base/allocator/allocator_interception_mac.mm
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9942e23fec9e2bca8ebd70f79feb2120943b54b commit e9942e23fec9e2bca8ebd70f79feb2120943b54b Author: ajuma <ajuma@chromium.org> Date: Thu Jan 26 16:35:31 2017 Revert of Move logic from memory_mac to allocator_interception_mac. (patchset #5 id:80001 of https://codereview.chromium.org/2649973003/ ) Reason for revert: This seems to be causing compile to fail on the Mac GPU FYI Asan bot, with error: ../../base/allocator/allocator_interception_mac.mm:320:3: error: use of undeclared identifier 'DeprotectMallocZone' DeprotectMallocZone(zone, &reprotection_start, &reprotection_length, ^ 1 error generated. See https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Release/builds/1407/steps/compile/logs/stdio and https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Release?numbuilds=100 Original issue's description: > Move logic from memory_mac to allocator_interception_mac. > > This CL is a refactor and has no intended behavior changes. > > This CL moves the allocator interception logic from base/process/memory_mac.mm > to base/allocator/allocator_interception_mac.mm in preparation for an > implementation of the allocator shim. > > BUG= 665567 > > Review-Url: https://codereview.chromium.org/2649973003 > Cr-Commit-Position: refs/heads/master@{#446234} > Committed: https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea46ec096230506 TBR=primiano@chromium.org,mark@chromium.org,erikchen@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 665567 Review-Url: https://codereview.chromium.org/2658083002 Cr-Commit-Position: refs/heads/master@{#446346} [modify] https://crrev.com/e9942e23fec9e2bca8ebd70f79feb2120943b54b/base/BUILD.gn [delete] https://crrev.com/cb3a30a6acb4e3e774f9c0d416bb90c270065364/base/allocator/allocator_interception_mac.h [delete] https://crrev.com/cb3a30a6acb4e3e774f9c0d416bb90c270065364/base/allocator/allocator_interception_mac.mm [modify] https://crrev.com/e9942e23fec9e2bca8ebd70f79feb2120943b54b/base/process/memory_mac.mm
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa commit 9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa Author: erikchen <erikchen@chromium.org> Date: Thu Jan 26 19:22:32 2017 Move logic from memory_mac to allocator_interception_mac. This CL is a refactor and has no intended behavior changes. This CL moves the allocator interception logic from base/process/memory_mac.mm to base/allocator/allocator_interception_mac.mm in preparation for an implementation of the allocator shim. BUG= 665567 Review-Url: https://codereview.chromium.org/2649973003 Cr-Original-Commit-Position: refs/heads/master@{#446234} Committed: https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea46ec096230506 Review-Url: https://codereview.chromium.org/2649973003 Cr-Commit-Position: refs/heads/master@{#446385} [modify] https://crrev.com/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa/base/BUILD.gn [add] https://crrev.com/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa/base/allocator/allocator_interception_mac.h [add] https://crrev.com/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa/base/allocator/allocator_interception_mac.mm [modify] https://crrev.com/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa/base/process/memory_mac.mm
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d774b55241a0a1d4c7f0eba23592c4b5de4ce24c commit d774b55241a0a1d4c7f0eba23592c4b5de4ce24c Author: erikchen <erikchen@chromium.org> Date: Thu Jan 26 20:56:24 2017 Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions. This CL has no intended behavior change. Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim to easily integrate with the existing malloc interception code. BUG= 665567 Review-Url: https://codereview.chromium.org/2650363002 Cr-Original-Commit-Position: refs/heads/master@{#446255} Committed: https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3ac4d0590f6ff Review-Url: https://codereview.chromium.org/2650363002 Cr-Commit-Position: refs/heads/master@{#446440} [modify] https://crrev.com/d774b55241a0a1d4c7f0eba23592c4b5de4ce24c/base/allocator/allocator_interception_mac.h [modify] https://crrev.com/d774b55241a0a1d4c7f0eba23592c4b5de4ce24c/base/allocator/allocator_interception_mac.mm
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8cf26ee794a218d356d508521c80bf1e7520076b commit 8cf26ee794a218d356d508521c80bf1e7520076b Author: erikchen <erikchen@chromium.org> Date: Fri Jan 27 17:08:34 2017 Remove unnecessary lines from chrome/app/framework.order. The symbols __ZnwmPv and __ZdlPvS_ are no longer present in the final binary. BUG= 665567 Review-Url: https://codereview.chromium.org/2647343008 Cr-Commit-Position: refs/heads/master@{#446698} [modify] https://crrev.com/8cf26ee794a218d356d508521c80bf1e7520076b/chrome/app/framework.order
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83d8f196d15bc31bfbce50ecc435016ba2a96bcf commit 83d8f196d15bc31bfbce50ecc435016ba2a96bcf Author: erikchen <erikchen@chromium.org> Date: Fri Jan 27 22:37:54 2017 Fold //base/allocator:unified_allocator_shim into //base:base. This CL is a refactor and has no intended behavior change. There is no reason to have a separate source set for //base/allocator:unified_allocator_shim, which introduces dependency cycles. BUG= 665567 Review-Url: https://codereview.chromium.org/2651043008 Cr-Commit-Position: refs/heads/master@{#446800} [modify] https://crrev.com/83d8f196d15bc31bfbce50ecc435016ba2a96bcf/base/BUILD.gn [modify] https://crrev.com/83d8f196d15bc31bfbce50ecc435016ba2a96bcf/base/allocator/BUILD.gn
,
Jan 27 2017
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d0395aeb25b9ab1422ab547f976d049a14e192f commit 0d0395aeb25b9ab1422ab547f976d049a14e192f Author: erikchen <erikchen@chromium.org> Date: Thu Feb 02 06:16:29 2017 Hook up allocator shim on mac. This CL should have no behavioral effect, since the relevant code is not enabled yet. allocator_shim_override_mac_symbols.h intercepts heap allocations using the newly introduced APIs in allocator_interception_mac.h and redirects them to the allocator shim. This eventually forwards to AllocatorDispatch::default_dispatch, which in turns calls the original malloc zone functions.. BUG= 665567 Review-Url: https://codereview.chromium.org/2658723007 Cr-Commit-Position: refs/heads/master@{#447705} [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/BUILD.gn [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_interception_mac.h [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_interception_mac.mm [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim.h [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_glibc.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc [add] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_mac_zoned_malloc.cc [add] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_mac_zoned_malloc.h [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_tcmalloc.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_default_dispatch_to_winheap.cc [add] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_override_mac_symbols.h [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/allocator/allocator_shim_unittest.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/debug/thread_heap_usage_tracker.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/debug/thread_heap_usage_tracker_unittest.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/base/trace_event/malloc_dump_provider.cc [modify] https://crrev.com/0d0395aeb25b9ab1422ab547f976d049a14e192f/build/config/allocator.gni
,
Feb 2 2017
This broke ASAN, see https://bugs.chromium.org/p/chromium/issues/detail?id=688014
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89e4e70ff72647ccc29b884b0965112a7153a4cf commit 89e4e70ff72647ccc29b884b0965112a7153a4cf Author: erikchen <erikchen@chromium.org> Date: Thu Feb 02 21:20:40 2017 Fix an ASAN compile error in AllocatorInterceptionMac. BUG= 665567 Review-Url: https://codereview.chromium.org/2674653004 Cr-Commit-Position: refs/heads/master@{#447841} [modify] https://crrev.com/89e4e70ff72647ccc29b884b0965112a7153a4cf/base/allocator/allocator_interception_mac.mm
,
Feb 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9ffc9c6104f4cc7ff5e62a94869f111c645106a commit e9ffc9c6104f4cc7ff5e62a94869f111c645106a Author: primiano <primiano@chromium.org> Date: Mon Feb 06 19:52:44 2017 profiler: decouple ThreadHeapUsageTracker from allocator shim internals Background: ----------- Until crrev.com/2658723007, the allocator shim was initialized at compile time and hence always ready to be used. After that CL, the shim requires a run-time initialization. The code in ThreadHeapUsageTracker currently assumes that the shim is initialized by directly calling into the shim chain even before a malloc/free call is intercepted. There are two possible solutions to this: 1. Initialize the shim before ThreadHeapUsageTracker is initialized. 2. Prevent ThreadHeapUsageTracker from directly entering the shim. (more discussion in https://codereview.chromium.org/2658723007/#msg53). 1. is a one liner but not particularly clean. ThreadHeapUsageTracker is lazily initialized when a Thread (the main thread) is created (in the TrackedObject construction). This would mean, in practice, putting the shim initialization in tracked_objects.cc which is quite obscure. This CL: -------- The approach used here is 2. The existing code had already the right sentinel logic to detect and prevent re-entrancy. This CL is just extending that to the dtor and just using new/delete to create the TLS object. This allows to initialize the shim in a less obscure place (e.g. content_main_runner). BUG= 665567 , 644385 TEST=Build with use_experimental_allocator_shim=true on Mac, run base_unittests Review-Url: https://codereview.chromium.org/2675433002 Cr-Commit-Position: refs/heads/master@{#448360} [modify] https://crrev.com/e9ffc9c6104f4cc7ff5e62a94869f111c645106a/base/debug/thread_heap_usage_tracker.cc
,
Feb 13 2017
,
Mar 3 2017
,
May 19 2017
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by primiano@chromium.org
, Nov 15 2016Summary: Port allocator shim to Mac OSX (was: "malloc" column doesn't get detailed background when --enable-heap-profiling is enabled.)