Todd Lipcon has posted comments on this change. Change subject: Replace uses of gutil/atomicops with c++11 atomics ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 318: if (next_rowset_idx_.compare_exchange_weak(current_rowset_idx, rowset_meta_idx)) { this should be under a lock and not require any atomics Line 599: Schema* old_schema = schema_.exchange(new_schema.release()); it's not really perf sensitive here so not a big deal, but memory_order_seq_cst is more expensive than Release_Store() since it involves an 'mfence' call http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/util/rwc_lock-test.cc File src/kudu/util/rwc_lock-test.cc: Line 90: while (!state->stop.load()) { should be relaxed (otherwise we're getting an extra barrier here that might obscure a TSAN race) same below http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/util/spinlock_profiling.cc File src/kudu/util/spinlock_profiling.cc: Line 280: CHECK(!g_profiling_enabled.exchange(true)); this is changing the semantics so it doesn't allow recursive enter/exit, let's not change semantics in this patch -- To view, visit http://gerrit.cloudera.org:8080/1842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249b64ff5efd6c8acbd2c6ff0811f66ff2c4d73f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
