Dan Hecht has uploaded a new patch set (#2). 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, 463 insertions(+), 400 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/73/2573/2 -- To view, visit http://gerrit.cloudera.org:8080/2573 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]>
