You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2023/05/04 20:01:27 UTC
[Impala-ASF-CR] PROTOTYPE IMPALA-11694: Convert gutil atomic ops to C++ atomics
Hello Michael Smith,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19185
to look at the new patch set (#4).
Change subject: PROTOTYPE IMPALA-11694: Convert gutil atomic ops to C++ atomics
......................................................................
PROTOTYPE IMPALA-11694: Convert gutil atomic ops to C++ atomics
Impala uses the gutil's implementation of atomic ops,
which has several port-specific versions. As new processors
come out and make improvements, these can require maintenance
over time. When testing on ARM, we noticed that a test
for a spinlock was dramatically slower (> 500x) and this
is likely due to improper atomics implementations.
The Chromium codebase switched to using the C++ atomics
package for these implementations a few years ago in
https://github.com/chromium/chromium/commit/57a4e4a50c673c25e9cdaab53e32f6e53aa0b574
This seems like a much more maintainable approach. On ARM,
this fixes the spinlock issue.
This is a prototype of adapting this change to the Impala
codebase. This still needs more work, but it has passed
Impala tests in the past.
Known remaining work:
1. This will be taken over to the Kudu codebase's gutil
as well, so we need to double-check that all APIs they
use are covered.
2. Kudu has support for OS X, so this will need to be tested
with x86_64 and the M1 chips.
3. It would be good to factor out the PauseCPU() function
to its own header so it isn't mixed in with atomics.
4. This has diverged quite a bit from the Chromium change,
so doing another pass going through why things became
different would be good.
Change-Id: I3d1e3f4e71988a6c464071c1cd8bdebce622e4b8
---
M be/src/gutil/atomicops-internals-arm64.h
D be/src/gutil/atomicops-internals-macosx.h
A be/src/gutil/atomicops-internals-portable.h
D be/src/gutil/atomicops-internals-powerpc.h
D be/src/gutil/atomicops-internals-tsan.h
M be/src/gutil/atomicops-internals-x86.h
M be/src/gutil/atomicops.h
D be/src/gutil/auxiliary/atomicops-internals-arm-generic.h
D be/src/gutil/auxiliary/atomicops-internals-arm-v6plus.h
D be/src/gutil/auxiliary/atomicops-internals-windows.h
10 files changed, 322 insertions(+), 3,158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/19185/4
--
To view, visit http://gerrit.cloudera.org:8080/19185
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d1e3f4e71988a6c464071c1cd8bdebce622e4b8
Gerrit-Change-Number: 19185
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>