Chromium Code Reviews Chromium Code Reviews
Issue
1884223002:
Revert of Enable allocator shim for Android (https://codereview.chromium.org/1875043003/) (Closed)
DescriptionRevert of Enable allocator shim for Android (https://codereview.chromium.org/1875043003/) (patchset #2 id:20001 of https://codereview.chromium.org/1875173002/ )
Reason for revert:
Causes re-entrancy issues in builds instrumented with -finstrument-functions.
See: crbug.com/602744
Original issue's description:
> Reland of Enable allocator shim for Android (crrev.com/1875043003)
>
> Reason for reland:
> The original CL was reverted by crrev.com/1881523003 because it broke
> clang builds. This CL contains the fix for clang (see diff between
> patch-set 1 and 2).
> The culprit of the fix is that LLVM libc++ uses _NOEXCEPT instead
> of glibc's __THROW. The absence of it was causing errors like the
> following:
>
> ../../base/allocator/allocator_shim_override_cpp_symbols.h:20:25: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
> SHIM_ALWAYS_EXPORT void operator delete(void* p) __THROW
>
> 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
> CC=dalecurtis@chromium.org
>
> Committed: https://crrev.com/88bf797383cd290d9bd23db0045c5fb23e010beb
> Cr-Commit-Position: refs/heads/master@{#386444}
TBR=thakis@chromium.org,primiano@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=550886, 598075, 602744
Committed: https://crrev.com/a383051b5f2e7d6576368aab7bc393d577350810
Cr-Commit-Position: refs/heads/master@{#387364}
Patch Set 1 #
Messages
Total messages: 9 (3 generated)
|
||||||||||||||||||||||||||||||||||||||||||||||
