impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Improve AtomicInt abstraction and implementation
Date Tue, 22 Mar 2016 01:00:58 GMT
Hello Henry Robinson,

I'd like you to reexamine a change.  Please visit

to look at the new patch set (#3).

Change subject: Improve AtomicInt abstraction and implementation

Improve AtomicInt abstraction and implementation


1) Don't use operator overloading. It makes it very difficult to reason
about code that uses atomics.  So it's better to have visible hints in
the code, and we generally prefer to avoid operator overloading anyway.
Also don't allow copy constructor for some reason.

Eliminating assignment overloading brought to light a bug in the use of
ProgressUpdater - some progress could have been dropped if it occurs
before the ProgressUpdater is re-constructed.

2) Make the memory ordering semantics well defined and documented.

3) Make all operators actually atomic. For example, in the old code,
'operator T()' and 'operator=' were not guaranteed to be atomic (though
on x64 it would be in practice - but again, memory ordering wasn't
defined because of no compiler barrier).

4) Read() was pessimistic and provided full barrier semantics even
though this is not what's generally needed.

5) Trim down the interface to the bare minimum. Better to keep something
as subtle as atomics as simple as possible. Some could be removed,
others were moved into the place that used them when the routines
weren't generally useful.

For now, let's stick to the operation/barrier combinations that are most
commonly needed rather than implement to the full set.  Later, if Impala
implements more complicated lock-free algorithms, we may need to expand
this set, but it's currently sufficient and best to keep things simple.

The new implementation is based on atomicops, which we depend on already
by SpinLock. Later we can consider using C++11 atomics to implement the
underlying value_.

Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923
M be/src/common/
M be/src/common/atomic.h
M be/src/exec/
M be/src/exec/hdfs-scan-node.h
M be/src/exec/
M be/src/gutil/atomicops-internals-x86.h
M be/src/rpc/
M be/src/rpc/rpc-trace.h
M be/src/runtime/
M be/src/runtime/
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/
M be/src/runtime/
M be/src/runtime/
M be/src/runtime/
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/
M be/src/runtime/
M be/src/runtime/mem-tracker.h
M be/src/runtime/
M be/src/scheduling/
M be/src/scheduling/query-resource-mgr.h
M be/src/util/counting-barrier.h
M be/src/util/
M be/src/util/
M be/src/util/internal-queue.h
M be/src/util/
M be/src/util/periodic-counter-updater.h
M be/src/util/
M be/src/util/progress-updater.h
M be/src/util/
M be/src/util/
M be/src/util/runtime-profile.h
34 files changed, 469 insertions(+), 400 deletions(-)

  git pull ssh:// refs/changes/73/2573/3
To view, visit
To unsubscribe, visit

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Dan Hecht <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>

View raw message