This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 2857208cc Fix synchronization of object pool free chunks (#3615)
2857208cc is described below
commit 2857208cc032d6578743b48c670e842b810ebfff
Author: nkysg <[email protected]>
AuthorDate: Mon Oct 26 10:41:08 2026 +0800
Fix synchronization of object pool free chunks (#3615)
* Fix synchronization of object pool free chunks
* Add free chunk count transition tests
* rm unnecessary comments
* update unit tests
---
src/butil/object_pool_inl.h | 11 ++++++++---
src/butil/resource_pool_inl.h | 11 ++++++++---
test/BUILD.bazel | 1 +
test/CMakeLists.txt | 1 +
test/Makefile | 1 +
test/object_pool_unittest.cpp | 42 +++++++++++++++++++++++++++++++++++++++++
test/resource_pool_unittest.cpp | 42 +++++++++++++++++++++++++++++++++++++++++
7 files changed, 103 insertions(+), 6 deletions(-)
diff --git a/src/butil/object_pool_inl.h b/src/butil/object_pool_inl.h
index caabca9b0..ea78ee5a0 100644
--- a/src/butil/object_pool_inl.h
+++ b/src/butil/object_pool_inl.h
@@ -357,6 +357,7 @@ public:
private:
ObjectPool() {
_free_chunks.reserve(OP_INITIAL_FREE_LIST_SIZE);
+ _nfree_chunks.store(0, butil::memory_order_relaxed);
pthread_mutex_init(&_free_chunks_mutex, nullptr);
#if defined(BUTIL_USE_ASAN) && \
!defined(BAIDU_CLEAR_OBJECT_POOL_AFTER_ALL_THREADS_QUIT)
@@ -536,9 +537,10 @@ private:
private:
bool pop_free_chunk(FreeChunk& c) {
- // Critical for the case that most return_object are called in
- // different threads of get_object.
- if (_free_chunks.empty()) {
+ // This relaxed load is only a hint that avoids contending on the
+ // mutex when the shared free list is empty. The list itself remains
+ // protected by _free_chunks_mutex below.
+ if (_nfree_chunks.load(butil::memory_order_relaxed) == 0) {
return false;
}
pthread_mutex_lock(&_free_chunks_mutex);
@@ -548,6 +550,7 @@ private:
}
DynamicFreeChunk* p = _free_chunks.back();
_free_chunks.pop_back();
+ _nfree_chunks.fetch_sub(1, butil::memory_order_relaxed);
pthread_mutex_unlock(&_free_chunks_mutex);
c.nfree = p->nfree;
memcpy(c.ptrs, p->ptrs, sizeof(*p->ptrs) * p->nfree);
@@ -565,6 +568,7 @@ private:
memcpy(p->ptrs, c.ptrs, sizeof(*c.ptrs) * c.nfree);
pthread_mutex_lock(&_free_chunks_mutex);
_free_chunks.push_back(p);
+ _nfree_chunks.fetch_add(1, butil::memory_order_relaxed);
pthread_mutex_unlock(&_free_chunks_mutex);
return true;
}
@@ -580,6 +584,7 @@ private:
std::vector<DynamicFreeChunk*> _free_chunks;
pthread_mutex_t _free_chunks_mutex;
+ butil::static_atomic<size_t> _nfree_chunks;
#ifdef BUTIL_OBJECT_POOL_NEED_FREE_ITEM_NUM
static butil::static_atomic<size_t> _global_nfree;
diff --git a/src/butil/resource_pool_inl.h b/src/butil/resource_pool_inl.h
index 57134c901..1c75a0867 100644
--- a/src/butil/resource_pool_inl.h
+++ b/src/butil/resource_pool_inl.h
@@ -388,6 +388,7 @@ public:
private:
ResourcePool() {
_free_chunks.reserve(RP_INITIAL_FREE_LIST_SIZE);
+ _nfree_chunks.store(0, butil::memory_order_relaxed);
pthread_mutex_init(&_free_chunks_mutex, nullptr);
}
@@ -519,9 +520,10 @@ private:
private:
bool pop_free_chunk(FreeChunk& c) {
- // Critical for the case that most return_object are called in
- // different threads of get_object.
- if (_free_chunks.empty()) {
+ // This relaxed load is only a hint that avoids contending on the
+ // mutex when the shared free list is empty. The list itself remains
+ // protected by _free_chunks_mutex below.
+ if (_nfree_chunks.load(butil::memory_order_relaxed) == 0) {
return false;
}
pthread_mutex_lock(&_free_chunks_mutex);
@@ -531,6 +533,7 @@ private:
}
DynamicFreeChunk* p = _free_chunks.back();
_free_chunks.pop_back();
+ _nfree_chunks.fetch_sub(1, butil::memory_order_relaxed);
pthread_mutex_unlock(&_free_chunks_mutex);
c.nfree = p->nfree;
memcpy(c.ids, p->ids, sizeof(*p->ids) * p->nfree);
@@ -548,6 +551,7 @@ private:
memcpy(p->ids, c.ids, sizeof(*c.ids) * c.nfree);
pthread_mutex_lock(&_free_chunks_mutex);
_free_chunks.push_back(p);
+ _nfree_chunks.fetch_add(1, butil::memory_order_relaxed);
pthread_mutex_unlock(&_free_chunks_mutex);
return true;
}
@@ -563,6 +567,7 @@ private:
std::vector<DynamicFreeChunk*> _free_chunks;
pthread_mutex_t _free_chunks_mutex;
+ butil::static_atomic<size_t> _nfree_chunks;
#ifdef BUTIL_RESOURCE_POOL_NEED_FREE_ITEM_NUM
static butil::static_atomic<size_t> _global_nfree;
diff --git a/test/BUILD.bazel b/test/BUILD.bazel
index a2c291eb7..2d331018b 100644
--- a/test/BUILD.bazel
+++ b/test/BUILD.bazel
@@ -120,6 +120,7 @@ TEST_BUTIL_SOURCES = [
"crc32c_unittest.cc",
"iobuf_unittest.cpp",
"object_pool_unittest.cpp",
+ "resource_pool_unittest.cpp",
"test_switches.cc",
"scoped_locale.cc",
"recordio_unittest.cpp",
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 4c3a09f52..d4348027a 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -132,6 +132,7 @@ SET(TEST_BUTIL_SOURCES
${CMAKE_CURRENT_SOURCE_DIR}/non_thread_safe_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/nullable_string16_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/object_pool_unittest.cpp
+ ${CMAKE_CURRENT_SOURCE_DIR}/resource_pool_unittest.cpp
${CMAKE_CURRENT_SOURCE_DIR}/observer_list_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/optional_unittest.cpp
${CMAKE_CURRENT_SOURCE_DIR}/platform_thread_unittest.cc
diff --git a/test/Makefile b/test/Makefile
index f4d5462e6..4d007f266 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -145,6 +145,7 @@ TEST_BUTIL_SOURCES = \
crc32c_unittest.cc \
iobuf_unittest.cpp \
object_pool_unittest.cpp \
+ resource_pool_unittest.cpp \
recordio_unittest.cpp \
test_switches.cc \
scoped_locale.cc \
diff --git a/test/object_pool_unittest.cpp b/test/object_pool_unittest.cpp
index 366b05224..10ad931ab 100644
--- a/test/object_pool_unittest.cpp
+++ b/test/object_pool_unittest.cpp
@@ -25,6 +25,7 @@
namespace {
struct MyObject {};
+struct FreeChunkCountObject {};
int nfoo_dtor = 0;
struct Foo {
@@ -51,6 +52,10 @@ template <> struct ObjectPoolFreeChunkMaxItem<MyObject> {
static size_t value() { return 5; }
};
+template <> struct ObjectPoolFreeChunkMaxItem<FreeChunkCountObject> {
+ static size_t value() { return 1; }
+};
+
template <> struct ObjectPoolValidator<Foo> {
static bool validate(const Foo* foo) {
return foo->x != 0;
@@ -243,6 +248,43 @@ TEST_F(ObjectPoolTest, get_perf) {
std::cout << describe_objects<SilentObj>() << std::endl;
}
+struct FreeChunkCountArgs {
+ FreeChunkCountObject* first;
+ FreeChunkCountObject* second;
+};
+
+void* add_two_free_chunks(void* arg) {
+ FreeChunkCountArgs* args = static_cast<FreeChunkCountArgs*>(arg);
+ args->first = get_object<FreeChunkCountObject>();
+ args->second = get_object<FreeChunkCountObject>();
+ return_object(args->first);
+ return_object(args->second);
+ return nullptr;
+}
+
+TEST_F(ObjectPoolTest, free_chunk_count_tracks_push_and_pop) {
+ // Keep the main LocalPool alive so worker teardown does not clear chunks.
+ FreeChunkCountObject* guard = get_object<FreeChunkCountObject>();
+ ASSERT_NE(nullptr, guard);
+ FreeChunkCountArgs args = { nullptr, nullptr };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr, add_two_free_chunks, &args));
+ ASSERT_EQ(0, pthread_join(tid, nullptr));
+
+ FreeChunkCountObject* first = get_object<FreeChunkCountObject>();
+ FreeChunkCountObject* second = get_object<FreeChunkCountObject>();
+ ASSERT_NE(nullptr, first);
+ ASSERT_NE(nullptr, second);
+ ASSERT_TRUE(first == args.first || first == args.second);
+ ASSERT_TRUE(second == args.first || second == args.second);
+ ASSERT_NE(first, second);
+
+ return_object(first);
+ return_object(second);
+ return_object(guard);
+ clear_objects<FreeChunkCountObject>();
+}
+
struct D { int val[1]; };
void* get_and_return_int(void*) {
diff --git a/test/resource_pool_unittest.cpp b/test/resource_pool_unittest.cpp
index 9bc27f134..d3f6fbf79 100644
--- a/test/resource_pool_unittest.cpp
+++ b/test/resource_pool_unittest.cpp
@@ -25,6 +25,7 @@
namespace {
struct MyObject {};
+struct FreeChunkCountObject {};
int nfoo_dtor = 0;
struct Foo {
@@ -51,6 +52,10 @@ template <> struct ResourcePoolFreeChunkMaxItem<MyObject> {
static size_t value() { return 5; }
};
+template <> struct ResourcePoolFreeChunkMaxItem<FreeChunkCountObject> {
+ static size_t value() { return 1; }
+};
+
template <> struct ResourcePoolValidator<Foo> {
static bool validate(const Foo* foo) {
return foo->x != 0;
@@ -277,6 +282,43 @@ TEST_F(ResourcePoolTest, get_perf) {
std::cout << describe_resources<SilentObj>() << std::endl;
}
+struct FreeChunkCountArgs {
+ ResourceId<FreeChunkCountObject> first;
+ ResourceId<FreeChunkCountObject> second;
+};
+
+void* add_two_free_chunks(void* arg) {
+ FreeChunkCountArgs* args = static_cast<FreeChunkCountArgs*>(arg);
+ get_resource(&args->first);
+ get_resource(&args->second);
+ return_resource(args->first);
+ return_resource(args->second);
+ return nullptr;
+}
+
+TEST_F(ResourcePoolTest, free_chunk_count_tracks_push_and_pop) {
+ // Keep the main LocalPool alive so worker teardown does not clear chunks.
+ ResourceId<FreeChunkCountObject> guard = { 0 };
+ ASSERT_NE(nullptr, get_resource(&guard));
+ FreeChunkCountArgs args = { { 0 }, { 0 } };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr, add_two_free_chunks, &args));
+ ASSERT_EQ(0, pthread_join(tid, nullptr));
+
+ ResourceId<FreeChunkCountObject> first = { 0 };
+ ResourceId<FreeChunkCountObject> second = { 0 };
+ ASSERT_NE(nullptr, get_resource(&first));
+ ASSERT_NE(nullptr, get_resource(&second));
+ ASSERT_TRUE(first == args.first || first == args.second);
+ ASSERT_TRUE(second == args.first || second == args.second);
+ ASSERT_NE(first, second);
+
+ return_resource(first);
+ return_resource(second);
+ return_resource(guard);
+ clear_resources<FreeChunkCountObject>();
+}
+
struct D { int val[1]; };
void* get_and_return_int(void*) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
