You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2016/03/17 17:34:35 UTC

[Impala-CR](cdh5-trunk) Improve AtomicInt abstraction and implementation

Dan Hecht has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/2573

Change subject: Improve AtomicInt abstraction and implementation
......................................................................

Improve AtomicInt abstraction and implementation

Improvements:

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/atomic-test.cc
M be/src/common/atomic.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/scanner-context.cc
M be/src/gutil/atomicops-internals-x86.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/scheduling/query-resource-mgr.cc
M be/src/scheduling/query-resource-mgr.h
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
M be/src/util/internal-queue-test.cc
M be/src/util/internal-queue.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/progress-updater.cc
M be/src/util/progress-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
34 files changed, 447 insertions(+), 392 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/73/2573/1
-- 
To view, visit http://gerrit.cloudera.org:8080/2573
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>