Repository: kudu Updated Branches: refs/heads/master bb78520a7 -> 3bb5f56cf
util: add once class based on std::call_once The existing KuduOnceDynamic class only allows calling functions of type Status(void). I intend on adding an argument to some of the existing the Status(void) methods that get called using a KuduOnceDynamic. As such, this patch adds a KuduOnceLambda class that accepts a lambda and otherwise has the same semantics as KuduOnceDynamic. I've replaced the KuduOnceDynamics that call methods that I intend on addding arguments to with their corresponding KuduOnceLambdas. Change-Id: Ide56057a800bf07923031df5a2a76a42f0b15358 Reviewed-on: http://gerrit.cloudera.org:8080/11302 Reviewed-by: Grant Henke <granthe...@apache.org> Tested-by: Andrew Wong <aw...@cloudera.com> Reviewed-by: Adar Dembo <a...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/54afea09 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/54afea09 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/54afea09 Branch: refs/heads/master Commit: 54afea093dd2f82aea6cc289e4f5409972c487b3 Parents: bb78520 Author: Andrew Wong <aw...@cloudera.com> Authored: Wed Aug 22 23:09:37 2018 -0700 Committer: Andrew Wong <aw...@cloudera.com> Committed: Tue Aug 28 00:21:53 2018 +0000 ---------------------------------------------------------------------- src/kudu/cfile/bloomfile.cc | 2 +- src/kudu/cfile/bloomfile.h | 2 +- src/kudu/cfile/cfile_reader.cc | 2 +- src/kudu/cfile/cfile_reader.h | 2 +- src/kudu/tablet/deltafile.cc | 2 +- src/kudu/tablet/deltafile.h | 2 +- src/kudu/util/once-test.cc | 57 ++++++++++++++++++++++++------------- src/kudu/util/once.cc | 8 ++++++ src/kudu/util/once.h | 47 +++++++++++++++++++++++++++--- 9 files changed, 94 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/cfile/bloomfile.cc ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/bloomfile.cc b/src/kudu/cfile/bloomfile.cc index b3628ee..0dbfa63 100644 --- a/src/kudu/cfile/bloomfile.cc +++ b/src/kudu/cfile/bloomfile.cc @@ -233,7 +233,7 @@ BloomFileReader::BloomFileReader(unique_ptr<CFileReader> reader, } Status BloomFileReader::Init() { - return init_once_.Init(&BloomFileReader::InitOnce, this); + return init_once_.Init([this] { return InitOnce(); }); } Status BloomFileReader::InitOnce() { http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/cfile/bloomfile.h ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/bloomfile.h b/src/kudu/cfile/bloomfile.h index 6e7b280..e1553ec 100644 --- a/src/kudu/cfile/bloomfile.h +++ b/src/kudu/cfile/bloomfile.h @@ -149,7 +149,7 @@ class BloomFileReader { std::unique_ptr<CFileReader> reader_; - KuduOnceDynamic init_once_; + KuduOnceLambda init_once_; ScopedTrackedConsumption mem_consumption_; }; http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/cfile/cfile_reader.cc ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/cfile_reader.cc b/src/kudu/cfile/cfile_reader.cc index 3211751..e0d7688 100644 --- a/src/kudu/cfile/cfile_reader.cc +++ b/src/kudu/cfile/cfile_reader.cc @@ -192,7 +192,7 @@ Status CFileReader::InitOnce() { } Status CFileReader::Init() { - RETURN_NOT_OK_PREPEND(init_once_.Init(&CFileReader::InitOnce, this), + RETURN_NOT_OK_PREPEND(init_once_.Init([this] { return InitOnce(); }), Substitute("failed to init CFileReader for block $0", block_id().ToString())); return Status::OK(); http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/cfile/cfile_reader.h ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/cfile_reader.h b/src/kudu/cfile/cfile_reader.h index 58fb60f..7263d3f 100644 --- a/src/kudu/cfile/cfile_reader.h +++ b/src/kudu/cfile/cfile_reader.h @@ -214,7 +214,7 @@ class CFileReader { const TypeInfo *type_info_; const TypeEncodingInfo *type_encoding_info_; - KuduOnceDynamic init_once_; + KuduOnceLambda init_once_; ScopedTrackedConsumption mem_consumption_; }; http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/tablet/deltafile.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/deltafile.cc b/src/kudu/tablet/deltafile.cc index 4cd832f..805483c 100644 --- a/src/kudu/tablet/deltafile.cc +++ b/src/kudu/tablet/deltafile.cc @@ -255,7 +255,7 @@ DeltaFileReader::DeltaFileReader(unique_ptr<CFileReader> cf_reader, delta_type_(delta_type) {} Status DeltaFileReader::Init() { - return init_once_.Init(&DeltaFileReader::InitOnce, this); + return init_once_.Init([this] { return InitOnce(); }); } Status DeltaFileReader::InitOnce() { http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/tablet/deltafile.h ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/deltafile.h b/src/kudu/tablet/deltafile.h index e1e418f..a7203c3 100644 --- a/src/kudu/tablet/deltafile.h +++ b/src/kudu/tablet/deltafile.h @@ -219,7 +219,7 @@ class DeltaFileReader : public DeltaStore, // The type of this delta, i.e. UNDO or REDO. const DeltaType delta_type_; - KuduOnceDynamic init_once_; + KuduOnceLambda init_once_; }; // Iterator over the deltas contained in a delta file. http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/util/once-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/once-test.cc b/src/kudu/util/once-test.cc index c0a79b1..58ebd2c 100644 --- a/src/kudu/util/once-test.cc +++ b/src/kudu/util/once-test.cc @@ -26,6 +26,7 @@ #include "kudu/util/once.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" +#include "kudu/util/test_util.h" #include "kudu/util/thread.h" using std::vector; @@ -35,15 +36,14 @@ namespace kudu { namespace { +template<class KuduOnceType> struct Thing { explicit Thing(bool should_fail) : should_fail_(should_fail), value_(0) { } - Status Init() { - return once_.Init(&Thing::InitOnce, this); - } + Status Init(); Status InitOnce() { if (should_fail_) { @@ -55,14 +55,40 @@ struct Thing { const bool should_fail_; int value_; - KuduOnceDynamic once_; + KuduOnceType once_; }; -} // anonymous namespace +template<> +Status Thing<KuduOnceDynamic>::Init() { + return once_.Init(&Thing<KuduOnceDynamic>::InitOnce, this); +} + +template<> +Status Thing<KuduOnceLambda>::Init() { + return once_.Init([this] { return InitOnce(); }); +} + +template<class KuduOnceType> +static void InitOrGetInitted(Thing<KuduOnceType>* t, int i) { + if (i % 2 == 0) { + LOG(INFO) << "Thread " << i << " initting"; + t->Init(); + } else { + LOG(INFO) << "Thread " << i << " value: " << t->once_.init_succeeded(); + } +} -TEST(TestOnce, KuduOnceDynamicTest) { +} // anonymous namespace + +typedef ::testing::Types<KuduOnceDynamic, KuduOnceLambda> KuduOnceTypes; +TYPED_TEST_CASE(TestOnce, KuduOnceTypes); + +template<class KuduOnceType> +class TestOnce : public KuduTest {}; + +TYPED_TEST(TestOnce, KuduOnceTest) { { - Thing t(false); + Thing<TypeParam> t(false); ASSERT_EQ(0, t.value_); ASSERT_FALSE(t.once_.init_succeeded()); @@ -74,7 +100,7 @@ TEST(TestOnce, KuduOnceDynamicTest) { } { - Thing t(true); + Thing<TypeParam> t(true); for (int i = 0; i < 2; i++) { ASSERT_TRUE(t.Init().IsIllegalState()); ASSERT_EQ(0, t.value_); @@ -83,17 +109,8 @@ TEST(TestOnce, KuduOnceDynamicTest) { } } -static void InitOrGetInitted(Thing* t, int i) { - if (i % 2 == 0) { - LOG(INFO) << "Thread " << i << " initting"; - t->Init(); - } else { - LOG(INFO) << "Thread " << i << " value: " << t->once_.init_succeeded(); - } -} - -TEST(TestOnce, KuduOnceDynamicThreadSafeTest) { - Thing thing(false); +TYPED_TEST(TestOnce, KuduOnceThreadSafeTest) { + Thing<TypeParam> thing(false); // The threads will read and write to thing.once_.initted. If access to // it is not synchronized, TSAN will flag the access as data races. @@ -101,7 +118,7 @@ TEST(TestOnce, KuduOnceDynamicThreadSafeTest) { for (int i = 0; i < 10; i++) { scoped_refptr<Thread> t; ASSERT_OK(Thread::Create("test", Substitute("thread $0", i), - &InitOrGetInitted, &thing, i, &t)); + &InitOrGetInitted<TypeParam>, &thing, i, &t)); threads.push_back(t); } http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/util/once.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/once.cc b/src/kudu/util/once.cc index fada777..867c73e 100644 --- a/src/kudu/util/once.cc +++ b/src/kudu/util/once.cc @@ -29,4 +29,12 @@ size_t KuduOnceDynamic::memory_footprint_including_this() const { return kudu_malloc_usable_size(this) + memory_footprint_excluding_this(); } +size_t KuduOnceLambda::memory_footprint_excluding_this() const { + return status_.memory_footprint_excluding_this(); +} + +size_t KuduOnceLambda::memory_footprint_including_this() const { + return kudu_malloc_usable_size(this) + memory_footprint_excluding_this(); +} + } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/54afea09/src/kudu/util/once.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/once.h b/src/kudu/util/once.h index 0f43064..32c426c 100644 --- a/src/kudu/util/once.h +++ b/src/kudu/util/once.h @@ -14,11 +14,12 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -#ifndef KUDU_UTIL_ONCE_H -#define KUDU_UTIL_ONCE_H +#pragma once #include <stddef.h> +#include <mutex> + #include "kudu/gutil/once.h" #include "kudu/gutil/port.h" #include "kudu/util/atomic.h" @@ -111,6 +112,44 @@ class KuduOnceDynamic { Status status_; }; -} // namespace kudu +// Similar to the KuduOnceDynamic class, but accepts a lambda function. +class KuduOnceLambda { + public: + KuduOnceLambda() + : init_succeeded_(false) {} + + // If the underlying `once_flag` has yet to be invoked, invokes the provided + // lambda and stores its return value. Otherwise, returns the stored Status. + template<typename Fn> + Status Init(Fn fn) { + std::call_once(once_flag_, [this, fn] { + status_ = fn(); + if (PREDICT_TRUE(status_.ok())) { + init_succeeded_.Store(true, kMemOrderRelease); + } + }); + return status_; + } + + // Similar to KuduOnceDynamic, kMemOrderAcquire here and kMemOrderRelease in + // Init(), taken together, mean that threads can safely synchronize on + // ini_succeeded_. + bool init_succeeded() const { + return init_succeeded_.Load(kMemOrderAcquire); + } + + // Returns the memory usage of this object without the object itself. Should + // be used when embedded inside another object. + size_t memory_footprint_excluding_this() const; -#endif + // Returns the memory usage of this object including the object itself. + // Should be used when allocated on the heap. + size_t memory_footprint_including_this() const; + + private: + AtomicBool init_succeeded_; + std::once_flag once_flag_; + Status status_; +}; + +} // namespace kudu