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>