From 35f5ffe8dad0a545b6c0f95ac7236cf5c26748af Mon Sep 17 00:00:00 2001 From: primiano Date: Fri, 15 Apr 2016 07:14:26 -0700 Subject: [PATCH] Reland (2) of Enable allocator shim for Android (crrev.com/1875043003) Reason for reland: The CL was re-reverted by crrev.com/1884223002 because it broke the internal orderfile bot. The reason of the breakage was the following: - cygprofile.cc instruments all function calls with a __cyg_profile_func_enter preamble. - __cyg_profile_func_enter uses __thread. __thread under the hoods invokes calloc(), on every thread, to initialize the TLS. - The shim layer provides its own implementation of calloc(). - At this point, calloc() gets instrumented as well and it re-enters __cyg_profile_func_enter causing an infinite loop. The key of the problem here is that __thread silently causes calls to calloc() in a way that is out of control of cygprofile.cc. The solution proposed by this CL is the following: - Don't use __thread, use explicit POSIX functions for TLS (also there doesn't seem to be any precendent of using __thread in the codebase). - Use a global variable to prevent re-entrancy of the __cyg_profile_func_enter in the global (once per process) TLS slot initializer. - Re-entrancy is gone. Original issue's description: > Enable allocator shim for Android > > This is a follow-up to crrev.com/1719433002, which introduced the > shim for Android, and enables it by default by setting > use_experimental_allocator_shim=true for Android. > > Build/Perf sheriffs heads up > ---------------------------- > If you see any build error or crash related with __wrap_malloc, > __wrap_free, __real_malloc, __real_free, etc this CL is to blame. > > Performance considerations > ------------------------ > Binary size diff (GN, arm, static, official build): 24k > > I did a mixture of local and trybots run to estimate the perf impact > of this change. Didn't get any conclusive data, everything I tried > seems in the same ballpark, below noise levels. More in details: > > cc_perftests.PrepareTiles on a Nexus 4. > Rationale of the choice: in a previous CL (crbug.com/593344), this > benchmark revealed the presence of two mfences in the malloc path. > Results: https://goo.gl/8VC3Jp in the same ballpark. > > page-cycler on Nexus 9 via trybots: > Results: http://goo.gl/J3i50a seems to suggest that this CL improves > both warm and cold times in most cases. I doubt it, more likely it's > noise. > > All the other perf trybots failed. The perf waterfall seems to be in a > bad state in these days. > > BUG=550886,598075 > TEST=base_unittests --gtest_filter=AllocatorShimTest.* > TBR=thakis@chromium.org > > Committed: https://crrev.com/ebb95496c73dc0d5ce83968ac619921f154305f7 > Cr-Commit-Position: refs/heads/master@{#386386} BUG=550886,598075, 602744 TBR=thakis@chromium.org TEST=gn gen out/Debug --args='is_debug=true target_os="android" use_order_profiling=true target_cpu="arm" is_clang=false'; ninja -C out/Debug/ cygprofile_unittests; adb push out/Debug/cygprofile_unittests /data/local/tmp/cygprofile_unittests_debug; adb shell /data/local/tmp/cygprofile_unittests_debug Review URL: https://codereview.chromium.org/1883093005 Cr-Original-Commit-Position: refs/heads/master@{#387594} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: f7a321facfdabd763ecdbc9536c890fe91c8c079 --- common.gypi | 2 +- config/allocator.gni | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common.gypi b/common.gypi index a85d04a38..a3b505313 100644 --- a/common.gypi +++ b/common.gypi @@ -2274,7 +2274,7 @@ 'use_allocator%': 'none', 'use_sanitizer_options%': 1, }], - ['OS=="linux" and asan==0 and msan==0 and lsan==0 and tsan==0 and build_for_tool==""', { + ['(OS=="linux" or OS=="android") and asan==0 and msan==0 and lsan==0 and tsan==0 and build_for_tool==""', { 'use_experimental_allocator_shim%': 1, }], ['OS=="linux" and asan==0 and msan==0 and lsan==0 and tsan==0', { diff --git a/config/allocator.gni b/config/allocator.gni index b6444a2a7..06447af0b 100644 --- a/config/allocator.gni +++ b/config/allocator.gni @@ -11,7 +11,7 @@ if (is_android || current_cpu == "mipsel" || is_mac || is_ios || is_asan || _default_allocator = "tcmalloc" } -if (is_linux && !is_asan && !is_lsan && !is_tsan && !is_msan) { +if ((is_linux || is_android) && !is_asan && !is_lsan && !is_tsan && !is_msan) { _default_use_experimental_allocator_shim = true } else { _default_use_experimental_allocator_shim = false