You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2023/05/09 14:52:53 UTC

[Impala-ASF-CR] IMPALA-11694: Convert gutil atomic ops to C++ atomics

spop@amazon.com has uploaded a new patch set (#7) to the change originally created by Joe McDonnell. ( http://gerrit.cloudera.org:8080/19185 )

Change subject: IMPALA-11694: Convert gutil atomic ops to C++ atomics
......................................................................

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.

The patch moves both the x86 and arm64 implementation
to a common file yield_processor.h following the structure
in the chromium project:
https://github.com/chromium/chromium/blame/main/base/allocator/partition_allocator/yield_processor.h

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.

Change-Id: I3d1e3f4e71988a6c464071c1cd8bdebce622e4b8
---
M be/src/common/init.cc
M be/src/gutil/CMakeLists.txt
D 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
D be/src/gutil/atomicops-internals-x86.cc
D 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
A be/src/gutil/yield_processor.h
14 files changed, 359 insertions(+), 3,390 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/19185/7
-- 
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: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sp...@amazon.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>