Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676093003:
mac: Hook up allocator shim during app startup. (Closed)
Descriptionmac: Hook up allocator shim during app startup and for tests.
This CL has no intended functional change.
The flag to flip the allocator shim to "true" will be set in another CL. This
plumbing is being landed separately, since flipping the flag is expected to be
disruptive and will likely need to go through several land/revert cycles.
BUG=677302
Review-Url: https://codereview.chromium.org/2676093003
Cr-Commit-Position: refs/heads/master@{#449508}
Committed: https://chromium.googlesource.com/chromium/src/+/bf11b55a9f6874eed37da83fe741494e39e4b3b9
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments from primiano. #
Total comments: 10
Patch Set 3 : Comments from primiano. #Patch Set 4 : Remove unused include. #
Messages
Total messages: 23 (13 generated)
erikchen@chromium.org changed reviewers: + avi@chromium.org, primiano@chromium.org
avi: Please review.
The CQ bit was checked by erikchen@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...
lgtm
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_TIMED_OUT, no build URL)
LGTM with some comments https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... content/app/content_main_runner.cc:90: #include "base/allocator/allocator_shim.h" I never if you can rely on the transitive dependency on the buildflag target (content -> base -> base/allocator:features) or you need a direct dependency from content -> allocator:features. Can you check that if you do: ninja -C out/whatever object/content/app/content_main_runner.o (or whatever is the right path) rm out/whatever/gen/base/allocator/features.h ninja -C out/whatever object/content/app/content_main_runner.o everything works? https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... content/app/content_main_runner.cc:483: base::EnableTerminationOnOutOfMemory(); I think you need to update EnableTerminationOnOutOfMemory in memory_mac.mm and do: #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) allocator::SetCallNewHandlerOnMallocFailure(true); #endif similarly to what happens in memory_linux.cc Otherwise I think that when you enable the shim you will lose the suicide-on-oom for the default malloc zone. I'd expect this to make the OutOfMemoryDeathTest in memory_unittest.cc fail (alright, the build flag is not enabled by default right now, that's why you won't see this unless you run manually base_unittest). If you run base_unittests do you see the failure? If yes: it's all great, fix EnableTerminationOnOOM If no: Option A: I am misunderstanding something Option B: the OutOfMemoryDeathTest are broken on mac and we should figure out why
On 2017/02/06 10:19:10, Primiano Tucci wrote: > LGTM with some comments > > https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... > content/app/content_main_runner.cc:90: #include > "base/allocator/allocator_shim.h" > I never if you can rely on the transitive dependency on the buildflag target > (content -> base -> base/allocator:features) or you need a direct dependency > from content -> allocator:features. > Can you check that if you do: > ninja -C out/whatever object/content/app/content_main_runner.o (or whatever is > the right path) > rm out/whatever/gen/base/allocator/features.h > ninja -C out/whatever object/content/app/content_main_runner.o > > everything works? Seems fine to me. With _default_use_experimental_allocator_shim = true: """ erikchen@erikchen-macpro ~/projects/chromium/src (temp72)$ rm out/gn/obj/content/app/content_main_runner_both/content_main_runner.o erikchen@erikchen-macpro ~/projects/chromium/src (temp72)$ ninja -C out/gn -j 50 -k 1000 obj/content/app/content_main_runner_both/content_main_runner.o ninja: Entering directory `out/gn' [1/1] CXX obj/content/app/content_main_runner_both/content_main_runner.o erikchen@erikchen-macpro ~/projects/chromium/src (temp72)$ rm out/gn/gen/base/allocator/features.h erikchen@erikchen-macpro ~/projects/chromium/src (temp72)$ ninja -C out/gn -j 50 -k 1000 obj/content/app/content_main_runner_both/content_main_runner.o ninja: Entering directory `out/gn' [4/4] CXX obj/content/app/content_main_runner_both/content_main_runner.o """
Description was changed from ========== mac: Hook up allocator shim during app startup. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. BUG=677302 ========== to ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. The plumbing is being finished separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ==========
Description was changed from ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. The plumbing is being finished separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ========== to ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. This plumbing is being finished separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ==========
Description was changed from ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. This plumbing is being finished separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ========== to ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. This plumbing is being landed separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ==========
erikchen@chromium.org changed reviewers: + mark@chromium.org
primiano: Please review. mark: Please review base/ https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... content/app/content_main_runner.cc:90: #include "base/allocator/allocator_shim.h" On 2017/02/06 10:19:10, Primiano Tucci wrote: > I never if you can rely on the transitive dependency on the buildflag target > (content -> base -> base/allocator:features) or you need a direct dependency > from content -> allocator:features. > Can you check that if you do: > ninja -C out/whatever object/content/app/content_main_runner.o (or whatever is > the right path) > rm out/whatever/gen/base/allocator/features.h > ninja -C out/whatever object/content/app/content_main_runner.o > > everything works? Done. https://codereview.chromium.org/2676093003/diff/1/content/app/content_main_ru... content/app/content_main_runner.cc:483: base::EnableTerminationOnOutOfMemory(); On 2017/02/06 10:19:10, Primiano Tucci wrote: > I think you need to update EnableTerminationOnOutOfMemory in memory_mac.mm and > do: > > #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) > allocator::SetCallNewHandlerOnMallocFailure(true); > #endif > > similarly to what happens in memory_linux.cc > > Otherwise I think that when you enable the shim you will lose the suicide-on-oom > for the default malloc zone. > I'd expect this to make the OutOfMemoryDeathTest in memory_unittest.cc fail > (alright, the build flag is not enabled by default right now, that's why you > won't see this unless you run manually base_unittest). > If you run base_unittests do you see the failure? > If yes: it's all great, fix EnableTerminationOnOOM > If no: > Option A: I am misunderstanding something > Option B: the OutOfMemoryDeathTest are broken on mac and we should figure out > why > > Tests were passing because we weren't initializing the allocator shim in tests. I've added the requisite plumbing and locally, tests pass with both _default_use_experimental_allocator_shim=true and _default_use_experimental_allocator_shim=false
LGTM
some final comments, I think the latest patchset can be cleaned up a bit. https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_check.cc (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_check.cc:36: #elif defined(OS_MACOSX) && BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) I think this should really be: #elif defined(OS_MACOSX) && !defined(MEMORY_TOOL_REPLACES_ALLOCATOR) return base::allocator::g_replaced_default_zone // (from allocator_interception_mac.mm) #endif note how I voluntarily omitted USE_EXPERIMENTAL_ALLOCATOR_SHIM. Rationale? This function really checks that the allocator hooks are armed, so that the suicide on OOM will work if enabled via EnabledTerminationOnOOM. On OSX there are two ways to achieve this: 1) current state, without the shim: InterceptAllocationsMac replaces all the zones including the default one (InterceptAllocationsMac() -> ReplaceFunctionsForDefaultZone()) 2) future state, with the shime: the shim layer will use StoreFunctionsForDefaultZone to setup itself (InitializeAllocatorShim() -> OverrideMacSymbols() -> ReplaceFunctionsForDefaultZone) https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:451: std::set_new_handler(oom_killer_new); (read this comment after the one below) Honestly I think this is the misplaced thing that mislead you. I know that you didn't touch this, but I think that this call should really happen on memory_mac.mm -> EnableTerminationOnOutOfMemory(), similarly to what happens in Linux. In other words, I think it would be cleaner if memory_mac.mm looked like this: EnableTerminationOnOutOfMemory() { // Step 1, enable OOM killer on C++ failures std::set_new_handler(oom_killer_new); // Step 2: enabled OOM killer on C-malloc failures for the default zone (if we have a shim) #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) allocator::SetCallNewHandlerOnMallocFailure(true); #endif // Step 3: enable OOM killer on all other malloc zones (or just "all" without "other" if shim is disabled) allocator::InterceptAllocationsMac(); // (and this is the reason why in the previous CL I was saying that this has a bad name now.. but we'll deal with naming later) } Other than being cleaner, as a side effect this will look more consistent with memory_linux.cc EnableTerminationOnOutOfMemory() https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:454: allocator::SetCallNewHandlerOnMallocFailure(true); why here and not in memory_mac.mm -> EnableTerminationOnOutOfMemory() ? That would be consistent with what we do today in memory_linux.cc. this is creating a logic dependency cycle. So far, allocator_interception_mac.mm has been a low level building block, which knows how to deal with malloc replacement stuff * allocator_interception_mac.mm has no idea what the shim layer is. the shim layer uses allocator_interception_mac.mm to implement malloc shimming on OSX. Now with this line you are creating the following situation: - the shim layer knows about allocator_interception_mac.mm and uses it to insert itself in the malloc zone. - allocator_interception_mac.mm knows about the shim and uses it to turn on the suicide-on-oom flag So if you do this, and look at the full picture, the end-to-end stacktrace will look as follows (i hope that rietveld won't screw up my indentation ascii art with non-monospace fonts): content_main_runner.cc ContentMainRunnerImpl::Initialize() allocator_shim.cc InitializeAllocatorShim() allocator_interception_mac.mm ReplaceFunctionsForDefaultZone() allocator_shim.cc SetCallNewHandlerOnMallocFailure() memory_mac.mm EnableTerminationOnOutOfMemory() allocator_interception_mac.mm InterceptAllocationsMac() (note the allocator_shim -> interception_mac -> allocator_shim sequence) Instead what I was proposing is: content_main_runner.cc ContentMainRunnerImpl::Initialize() allocator_shim.cc InitializeAllocatorShim() allocator_interception_mac.mm ReplaceFunctionsForDefaultZone() memory_mac.mm EnableTerminationOnOutOfMemory() allocator_interception_mac.mm InterceptAllocationsMac() allocator_shim.cc SetCallNewHandlerOnMallocFailure() which looks a bit more cleaner to me. * There is indeed a non-ideal thing in all this, the fact that the suicide-on-OOM for the non-default zones happens in allocator_interception_mac.mm while the one for the default zone happens in the shim. but, as we discussed in the past, we have to live with it and the only other option is refactoring all the shim to introduce the notions of zones (which is probably going to create more headaches). https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_shim.h:118: extern bool g_is_mac_shim_layer_initialized; see comment above, I don't think we need this https://codereview.chromium.org/2676093003/diff/20001/base/process/memory_uni... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/2676093003/diff/20001/base/process/memory_uni... base/process/memory_unittest.cc:125: #if defined(OS_MACOSX) && BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) I think you just need to move these 3 lines (#if...#endif) below in OutOfMemoryDeathTest:: SetUpInDeathAssert()
https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_check.cc (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_check.cc:36: #elif defined(OS_MACOSX) && BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) On 2017/02/07 11:58:08, Primiano Tucci wrote: > I think this should really be: > #elif defined(OS_MACOSX) && !defined(MEMORY_TOOL_REPLACES_ALLOCATOR) > return base::allocator::g_replaced_default_zone // (from > allocator_interception_mac.mm) > #endif > > note how I voluntarily omitted USE_EXPERIMENTAL_ALLOCATOR_SHIM. > > Rationale? > This function really checks that the allocator hooks are armed, so that the > suicide on OOM will work if enabled via EnabledTerminationOnOOM. > On OSX there are two ways to achieve this: > 1) current state, without the shim: InterceptAllocationsMac replaces all the > zones including the default one (InterceptAllocationsMac() -> > ReplaceFunctionsForDefaultZone()) > 2) future state, with the shime: the shim layer will use > StoreFunctionsForDefaultZone to setup itself (InitializeAllocatorShim() -> > OverrideMacSymbols() -> ReplaceFunctionsForDefaultZone) Done. > This function really checks that the allocator hooks are armed, so that the > suicide on OOM will work if enabled via EnabledTerminationOnOOM. Thanks for the explanation. This wasn't clear from the name of the function, and there is no documentation. I didn't realize this was supposed to return true iff. suicide on OOM works correctly. https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:451: std::set_new_handler(oom_killer_new); On 2017/02/07 11:58:08, Primiano Tucci wrote: > (read this comment after the one below) > Honestly I think this is the misplaced thing that mislead you. I know that you > didn't touch this, but I think that this call should really happen on > memory_mac.mm -> EnableTerminationOnOutOfMemory(), similarly to what happens in > Linux. > In other words, I think it would be cleaner if memory_mac.mm looked like > this: > EnableTerminationOnOutOfMemory() { > // Step 1, enable OOM killer on C++ failures > std::set_new_handler(oom_killer_new); > > // Step 2: enabled OOM killer on C-malloc failures for the default zone (if we > have a shim) > #if BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) > allocator::SetCallNewHandlerOnMallocFailure(true); > #endif > > // Step 3: enable OOM killer on all other malloc zones (or just "all" without > "other" if shim is disabled) > allocator::InterceptAllocationsMac(); // (and this is the reason why in the > previous CL I was saying that this has a bad name now.. but we'll deal with > naming later) > } > > Other than being cleaner, as a side effect this will look more consistent with > memory_linux.cc EnableTerminationOnOutOfMemory() Done. https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:454: allocator::SetCallNewHandlerOnMallocFailure(true); On 2017/02/07 11:58:08, Primiano Tucci wrote: > why here and not in memory_mac.mm -> EnableTerminationOnOutOfMemory() ? > That would be consistent with what we do today in memory_linux.cc. > > this is creating a logic dependency cycle. So far, allocator_interception_mac.mm > has been a low level building block, which knows how to deal with malloc > replacement stuff * > allocator_interception_mac.mm has no idea what the shim layer is. the shim layer > uses allocator_interception_mac.mm to implement malloc shimming on OSX. > Now with this line you are creating the following situation: > - the shim layer knows about allocator_interception_mac.mm and uses it to insert > itself in the malloc zone. > - allocator_interception_mac.mm knows about the shim and uses it to turn on the > suicide-on-oom flag > So if you do this, and look at the full picture, the end-to-end stacktrace will > look as follows (i hope that rietveld won't screw up my indentation ascii art > with non-monospace fonts): > > content_main_runner.cc ContentMainRunnerImpl::Initialize() > allocator_shim.cc InitializeAllocatorShim() > allocator_interception_mac.mm ReplaceFunctionsForDefaultZone() > allocator_shim.cc SetCallNewHandlerOnMallocFailure() > memory_mac.mm EnableTerminationOnOutOfMemory() > allocator_interception_mac.mm InterceptAllocationsMac() > > (note the allocator_shim -> interception_mac -> allocator_shim sequence) > > Instead what I was proposing is: > > content_main_runner.cc ContentMainRunnerImpl::Initialize() > allocator_shim.cc InitializeAllocatorShim() > allocator_interception_mac.mm ReplaceFunctionsForDefaultZone() > memory_mac.mm EnableTerminationOnOutOfMemory() > allocator_interception_mac.mm InterceptAllocationsMac() > allocator_shim.cc SetCallNewHandlerOnMallocFailure() > > which looks a bit more cleaner to me. > > * There is indeed a non-ideal thing in all this, the fact that the > suicide-on-OOM for the non-default zones happens in > allocator_interception_mac.mm while the one for the default zone happens in the > shim. but, as we discussed in the past, we have to live with it and the only > other option is refactoring all the shim to introduce the notions of zones > (which is probably going to create more headaches). Done. https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim.h (right): https://codereview.chromium.org/2676093003/diff/20001/base/allocator/allocato... base/allocator/allocator_shim.h:118: extern bool g_is_mac_shim_layer_initialized; On 2017/02/07 11:58:08, Primiano Tucci wrote: > see comment above, I don't think we need this Done. https://codereview.chromium.org/2676093003/diff/20001/base/process/memory_uni... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/2676093003/diff/20001/base/process/memory_uni... base/process/memory_unittest.cc:125: #if defined(OS_MACOSX) && BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM) On 2017/02/07 11:58:08, Primiano Tucci wrote: > I think you just need to move these 3 lines (#if...#endif) below in > OutOfMemoryDeathTest:: SetUpInDeathAssert() Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, avi@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2676093003/#ps60001 (title: "Remove unused include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486684300010310,
"parent_rev": "55fad71126046d6d84bd763c34a45476444fce09", "commit_rev":
"bf11b55a9f6874eed37da83fe741494e39e4b3b9"}
Message was sent while issue was closed.
Description was changed from ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. This plumbing is being landed separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 ========== to ========== mac: Hook up allocator shim during app startup and for tests. This CL has no intended functional change. The flag to flip the allocator shim to "true" will be set in another CL. This plumbing is being landed separately, since flipping the flag is expected to be disruptive and will likely need to go through several land/revert cycles. BUG=677302 Review-Url: https://codereview.chromium.org/2676093003 Cr-Commit-Position: refs/heads/master@{#449508} Committed: https://chromium.googlesource.com/chromium/src/+/bf11b55a9f6874eed37da83fe741... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bf11b55a9f6874eed37da83fe741... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
