From 9b71299819c5d768cc704990947025a856f9821e Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Sun, 23 Feb 2025 14:28:08 +0800 Subject: [PATCH 1/4] Add a convenient and safe DoublyBufferedData::Read --- src/butil/containers/doubly_buffered_data.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/butil/containers/doubly_buffered_data.h b/src/butil/containers/doubly_buffered_data.h index 62f67eadc5..0460e84f2b 100644 --- a/src/butil/containers/doubly_buffered_data.h +++ b/src/butil/containers/doubly_buffered_data.h @@ -123,6 +123,12 @@ class DoublyBufferedData { // Returns 0 on success, -1 otherwise. int Read(ScopedPtr* ptr); + // fn is `int Fn(const T& foreground)', will be called with foreground instance. + // This function is not blocked by Read() and Modify() in other threads. + // Returns 0 on success, otherwise on error. + template + int Read(Fn& fn); + // Modify background and foreground instances. fn(T&, ...) will be called // twice. Modify() from different threads are exclusive from each other. // NOTE: Call same series of fn to different equivalent instances should @@ -570,6 +576,16 @@ int DoublyBufferedData::Read( return -1; } +template +template +int DoublyBufferedData::Read(Fn& fn) { + ScopedPtr ptr; + if (Read(&ptr) != 0) { + return -1; + } + return fn(*ptr); +} + template template size_t DoublyBufferedData::Modify(Fn& fn) { From 327dec7c22883f6be4c92a5242fe90a3f727cbc9 Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Mon, 24 Feb 2025 22:41:13 +0800 Subject: [PATCH 2/4] Optimize and add a UT --- src/butil/containers/doubly_buffered_data.h | 9 ++++++--- test/brpc_load_balancer_unittest.cpp | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/butil/containers/doubly_buffered_data.h b/src/butil/containers/doubly_buffered_data.h index 0460e84f2b..8819bfa463 100644 --- a/src/butil/containers/doubly_buffered_data.h +++ b/src/butil/containers/doubly_buffered_data.h @@ -127,7 +127,7 @@ class DoublyBufferedData { // This function is not blocked by Read() and Modify() in other threads. // Returns 0 on success, otherwise on error. template - int Read(Fn& fn); + int Read(Fn&& fn); // Modify background and foreground instances. fn(T&, ...) will be called // twice. Modify() from different threads are exclusive from each other. @@ -578,12 +578,15 @@ int DoublyBufferedData::Read( template template -int DoublyBufferedData::Read(Fn& fn) { +int DoublyBufferedData::Read(Fn&& fn) { + BAIDU_CASSERT((is_result_void::value), + "Fn must accept `const T&' and return void"); ScopedPtr ptr; if (Read(&ptr) != 0) { return -1; } - return fn(*ptr); + fn(*ptr); + return 0; } template diff --git a/test/brpc_load_balancer_unittest.cpp b/test/brpc_load_balancer_unittest.cpp index 509456e71c..c41895cdd8 100644 --- a/test/brpc_load_balancer_unittest.cpp +++ b/test/brpc_load_balancer_unittest.cpp @@ -80,6 +80,16 @@ bool AddN(Foo& f, int n) { return true; } +void read_cb(const Foo& f) { + ASSERT_EQ(0, f.x); +} + +struct CallableObj { + void operator()(const Foo& f) { + ASSERT_EQ(0, f.x); + } +}; + template void test_doubly_buffered_data() { // test doubly_buffered_data TLS limits @@ -97,6 +107,15 @@ void test_doubly_buffered_data() { ASSERT_EQ(0, d.Read(&ptr)); ASSERT_EQ(0, ptr->x); } + { + ASSERT_EQ(0, d.Read([](const Foo& f) { + ASSERT_EQ(0, f.x); + })); + ASSERT_EQ(0, d.Read(read_cb)); + ASSERT_EQ(0, d.Read(CallableObj())); + CallableObj co; + ASSERT_EQ(0, d.Read(co)); + } { typename DBD::ScopedPtr ptr; ASSERT_EQ(0, d.Read(&ptr)); From 4517d04df542490e28a04e3878cd97e96f58069b Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Mon, 24 Feb 2025 23:50:49 +0800 Subject: [PATCH 3/4] Support variadic functions of DoublyBufferedData --- src/butil/containers/doubly_buffered_data.h | 123 +++----------------- test/brpc_load_balancer_unittest.cpp | 6 +- 2 files changed, 21 insertions(+), 108 deletions(-) diff --git a/src/butil/containers/doubly_buffered_data.h b/src/butil/containers/doubly_buffered_data.h index 8819bfa463..59cc2559a7 100644 --- a/src/butil/containers/doubly_buffered_data.h +++ b/src/butil/containers/doubly_buffered_data.h @@ -123,7 +123,7 @@ class DoublyBufferedData { // Returns 0 on success, -1 otherwise. int Read(ScopedPtr* ptr); - // fn is `int Fn(const T& foreground)', will be called with foreground instance. + // `fn(const T&)' will be called with foreground instance. // This function is not blocked by Read() and Modify() in other threads. // Returns 0 on success, otherwise on error. template @@ -134,78 +134,15 @@ class DoublyBufferedData { // NOTE: Call same series of fn to different equivalent instances should // result in equivalent instances, otherwise foreground and background // instance will be inconsistent. - template size_t Modify(Fn& fn); - template size_t Modify(Fn& fn, const Arg1&); - template - size_t Modify(Fn& fn, const Arg1&, const Arg2&); + template + size_t Modify(Fn&& fn, Args&&... args); // fn(T& background, const T& foreground, ...) will be called to background // and foreground instances respectively. - template size_t ModifyWithForeground(Fn& fn); - template - size_t ModifyWithForeground(Fn& fn, const Arg1&); - template - size_t ModifyWithForeground(Fn& fn, const Arg1&, const Arg2&); + template + size_t ModifyWithForeground(Fn&& fn, Args&&... args); private: - template - struct WithFG0 { - WithFG0(Fn& fn, T* data) : _fn(fn), _data(data) { } - size_t operator()(T& bg) { - return _fn(bg, (const T&)_data[&bg == _data]); - } - private: - Fn& _fn; - T* _data; - }; - - template - struct WithFG1 { - WithFG1(Fn& fn, T* data, const Arg1& arg1) - : _fn(fn), _data(data), _arg1(arg1) {} - size_t operator()(T& bg) { - return _fn(bg, (const T&)_data[&bg == _data], _arg1); - } - private: - Fn& _fn; - T* _data; - const Arg1& _arg1; - }; - - template - struct WithFG2 { - WithFG2(Fn& fn, T* data, const Arg1& arg1, const Arg2& arg2) - : _fn(fn), _data(data), _arg1(arg1), _arg2(arg2) {} - size_t operator()(T& bg) { - return _fn(bg, (const T&)_data[&bg == _data], _arg1, _arg2); - } - private: - Fn& _fn; - T* _data; - const Arg1& _arg1; - const Arg2& _arg2; - }; - - template - struct Closure1 { - Closure1(Fn& fn, const Arg1& arg1) : _fn(fn), _arg1(arg1) {} - size_t operator()(T& bg) { return _fn(bg, _arg1); } - private: - Fn& _fn; - const Arg1& _arg1; - }; - - template - struct Closure2 { - Closure2(Fn& fn, const Arg1& arg1, const Arg2& arg2) - : _fn(fn), _arg1(arg1), _arg2(arg2) {} - size_t operator()(T& bg) { return _fn(bg, _arg1, _arg2); } - private: - Fn& _fn; - const Arg1& _arg1; - const Arg2& _arg2; - }; - const T* UnsafeRead() const { return _data + _index.load(butil::memory_order_acquire); } @@ -259,7 +196,8 @@ template class DoublyBufferedData::WrapperTLSGroup { public: const static size_t RAW_BLOCK_SIZE = 4096; - const static size_t ELEMENTS_PER_BLOCK = RAW_BLOCK_SIZE / sizeof(Wrapper) > 0 ? RAW_BLOCK_SIZE / sizeof(Wrapper) : 1; + const static size_t ELEMENTS_PER_BLOCK = + RAW_BLOCK_SIZE / sizeof(Wrapper) > 0 ? RAW_BLOCK_SIZE / sizeof(Wrapper) : 1; struct BAIDU_CACHELINE_ALIGNMENT ThreadBlock { inline DoublyBufferedData::Wrapper* at(size_t offset) { @@ -590,8 +528,8 @@ int DoublyBufferedData::Read(Fn&& fn) { } template -template -size_t DoublyBufferedData::Modify(Fn& fn) { +template +size_t DoublyBufferedData::Modify(Fn&& fn, Args&&... args) { // _modify_mutex sequences modifications. Using a separate mutex rather // than _wrappers_mutex is to avoid blocking threads calling // AddWrapper() or RemoveWrapper() too long. Most of the time, modifications @@ -600,7 +538,7 @@ size_t DoublyBufferedData::Modify(Fn& fn) { int bg_index = !_index.load(butil::memory_order_relaxed); // background instance is not accessed by other threads, being safe to // modify. - const size_t ret = fn(_data[bg_index]); + const size_t ret = fn(_data[bg_index], std::forward(args)...); if (!ret) { return 0; } @@ -626,46 +564,17 @@ size_t DoublyBufferedData::Modify(Fn& fn) { } } - const size_t ret2 = fn(_data[bg_index]); + const size_t ret2 = fn(_data[bg_index], std::forward(args)...); CHECK_EQ(ret2, ret) << "index=" << _index.load(butil::memory_order_relaxed); return ret2; } template -template -size_t DoublyBufferedData::Modify(Fn& fn, const Arg1& arg1) { - Closure1 c(fn, arg1); - return Modify(c); -} - -template -template -size_t DoublyBufferedData::Modify( - Fn& fn, const Arg1& arg1, const Arg2& arg2) { - Closure2 c(fn, arg1, arg2); - return Modify(c); -} - -template -template -size_t DoublyBufferedData::ModifyWithForeground(Fn& fn) { - WithFG0 c(fn, _data); - return Modify(c); -} - -template -template -size_t DoublyBufferedData::ModifyWithForeground(Fn& fn, const Arg1& arg1) { - WithFG1 c(fn, _data, arg1); - return Modify(c); -} - -template -template -size_t DoublyBufferedData::ModifyWithForeground( - Fn& fn, const Arg1& arg1, const Arg2& arg2) { - WithFG2 c(fn, _data, arg1, arg2); - return Modify(c); +template +size_t DoublyBufferedData::ModifyWithForeground(Fn&& fn, Args&&... args) { + return Modify([this, fn](T& bg, Args&&... args) { + return fn(bg, (const T&)_data[&bg == _data], std::forward(args)...); + }, std::forward(args)...); } } // namespace butil diff --git a/test/brpc_load_balancer_unittest.cpp b/test/brpc_load_balancer_unittest.cpp index c41895cdd8..2abb869ee4 100644 --- a/test/brpc_load_balancer_unittest.cpp +++ b/test/brpc_load_balancer_unittest.cpp @@ -123,10 +123,14 @@ void test_doubly_buffered_data() { } d.Modify(AddN, 10); + d.Modify([](Foo& f, int n) -> size_t { + f.x += n; + return 1; + }, 10); { typename DBD::ScopedPtr ptr; ASSERT_EQ(0, d.Read(&ptr)); - ASSERT_EQ(10, ptr->x); + ASSERT_EQ(20, ptr->x); } } From 52d5cc4a6e5d09637a868bb92ab3a16ffeb1cd57 Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Tue, 25 Feb 2025 22:19:52 +0800 Subject: [PATCH 4/4] Fix Fn copy --- src/butil/containers/doubly_buffered_data.h | 2 +- src/butil/type_traits.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/butil/containers/doubly_buffered_data.h b/src/butil/containers/doubly_buffered_data.h index 59cc2559a7..5c97038b42 100644 --- a/src/butil/containers/doubly_buffered_data.h +++ b/src/butil/containers/doubly_buffered_data.h @@ -572,7 +572,7 @@ size_t DoublyBufferedData::Modify(Fn&& fn, Args&& template template size_t DoublyBufferedData::ModifyWithForeground(Fn&& fn, Args&&... args) { - return Modify([this, fn](T& bg, Args&&... args) { + return Modify([this, &fn](T& bg, Args&&... args) { return fn(bg, (const T&)_data[&bg == _data], std::forward(args)...); }, std::forward(args)...); } diff --git a/src/butil/type_traits.h b/src/butil/type_traits.h index 0ae91135de..735e1e55ce 100644 --- a/src/butil/type_traits.h +++ b/src/butil/type_traits.h @@ -99,7 +99,7 @@ template struct is_pod std::is_trivial::value)> {}; #else template struct is_pod : std::is_pod {}; -#endif +#endif // __cplusplus #else // We can't get is_pod right without compiler help, so fail conservatively. @@ -334,7 +334,7 @@ template struct remove_cvref { typedef typename remove_cv::type>::type type; }; -#endif +#endif // __cplusplus // is_reference is false except for reference types. template struct is_reference : false_type {}; @@ -378,7 +378,7 @@ template using result_of = std::result_of; #else #error Only C++11 or later is supported. -#endif +#endif // __cplusplus template using result_of_t = typename result_of::type;