Use butil::ThreadLocal to store keytable #2645

MJY-HUST · 2024-05-19T07:18:10Z

What problem does this PR solve?

Issue Number:
#2635
Problem Summary:
当brpc server 下处理的应用执行时间较短(bthread生命周期短)，且使用了bthread_local变量时，由于keytable 由bthread_keytable_pool_t中的一个全局链表维护，borrow_keytable、return_keytable时内部加互斥锁，导致锁成为瓶颈。

基于此，重新设计了bthread_keytable_pool_t：
新的bthread_keytable_pool_t结构体中，使用butil::ThreadLocalbthread::KeyTableList* list 保存TLS的KeyTable list。同时保留原有的free_keytables，不改变reserved_thread_local_data的语义，调用bthread_keytable_pool_reserve会在全局链表中预分配keytable.原有的互斥锁改为读写锁。

borrow_keytable时优先从本地TLS链表中查找是否有可用的KeyTable，如果没有，且free_keytables不为NULL，再去从全局链表中加写锁获取keytable；如果都没有可用table，则返回NULL。
return_keytable时只会将keytable插入TLS的KeyTable list中，由于是TLS变量，只有当bthread_keytable_pool_destroy才会产生竞争，所以只需要加写锁。
bthread_keytable_pool_destroy时加写锁，首先delete butil::ThreadLocalbthread::KeyTableList* list，pool->destroyed = 1.然后如果free_keytables不为NULL，再释放free_keytables的空间。
borrow_keytable、return_keytable内部只加读锁，bthread_keytable_pool_destroy、bthread_keytable_pool_reserve内部加写锁。sever正常运行时，一般只会调用borrow_keytable、return_keytable接口，不会产生锁的竞争。

What is changed and the side effects?

Changed:

Side effects:

Performance effects(性能影响):
Breaking backward compatibility(向后兼容性):

Check List:

Please make sure your changes are compilable(请确保你的更改可以通过编译).
When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

chenBright · 2024-05-20T02:30:27Z

object_pool应该没法完全解决 #1449 的问题吧，从object_pool取出来的KeyTable有可能已经有data了，先调用bthread_setspecific函数，旧data还是会泄漏。

chenBright · 2024-05-20T02:34:22Z

brpc/src/brpc/server.h

Lines 170 to 183 in b601c89

 // The factory to create/destroy data attached to each searching thread 

 // in server. 

 // If this option is NULL, brpc::thread_local_data() is always NULL. 

 // NOT owned by Server and must be valid when Server is running. 

 // Default: NULL 

 const DataFactory* thread_local_data_factory; 

 // Prepare so many thread-local data before server starts, so that calls 

 // to brpc::thread_local_data() get data directly rather than calling 

 // thread_local_data_factory->Create() at first time. Useful when Create() 

 // is slow, otherwise the RPC session may be blocked by the creation 

 // of data and not served within timeout. 

 // Default: 0 

 size_t reserved_thread_local_data;

brpc/src/brpc/server.cpp

Lines 888 to 914 in b601c89

 // Init _keytable_pool always. If the server was stopped before, the pool 

 // should be destroyed in Join(). 

 _keytable_pool = new bthread_keytable_pool_t; 

 if (bthread_keytable_pool_init(_keytable_pool) != 0) { 

 LOG(ERROR) << "Fail to init _keytable_pool"; 

 delete _keytable_pool; 

 _keytable_pool = NULL; 

 return -1; 

 } 

 if (_options.thread_local_data_factory) { 

 _tl_options.thread_local_data_factory = _options.thread_local_data_factory; 

 if (bthread_key_create2(&_tl_options.tls_key, DestroyServerTLS, 

 _options.thread_local_data_factory) != 0) { 

 LOG(ERROR) << "Fail to create thread-local key"; 

 return -1; 

 } 

 if (_options.reserved_thread_local_data) { 

 bthread_keytable_pool_reserve(_keytable_pool, 

 _options.reserved_thread_local_data, 

 _tl_options.tls_key, 

 CreateServerTLS, 

 _options.thread_local_data_factory); 

 } 

 } else { 

 _tl_options = ThreadLocalOptions(); 

 }

应该没法废弃bthread_keytable_pool_t ，因为Server已经用了bthread_keytable_pool_t了，并对外提供了与之相关的设置项。

改造bthread_keytable_pool_t，将free_keytables由全局改成TLS，这样mutex就可以废弃了。同时修改reserved_thread_local_data语义：每个TLS上预留的data数量。这样可行吗？

MJY-HUST · 2024-05-20T12:28:14Z

brpc/src/brpc/server.h

Lines 170 to 183 in b601c89

// The factory to create/destroy data attached to each searching thread

// in server.

// If this option is NULL, brpc::thread_local_data() is always NULL.

// NOT owned by Server and must be valid when Server is running.

// Default: NULL

const DataFactory* thread_local_data_factory;

// Prepare so many thread-local data before server starts, so that calls

// to brpc::thread_local_data() get data directly rather than calling

// thread_local_data_factory->Create() at first time. Useful when Create()

// is slow, otherwise the RPC session may be blocked by the creation

// of data and not served within timeout.

// Default: 0

size_t reserved_thread_local_data;

brpc/src/brpc/server.cpp

Lines 888 to 914 in b601c89

// Init _keytable_pool always. If the server was stopped before, the pool

// should be destroyed in Join().

_keytable_pool = new bthread_keytable_pool_t;

if (bthread_keytable_pool_init(_keytable_pool) != 0) {

LOG(ERROR) << "Fail to init _keytable_pool";

delete _keytable_pool;

_keytable_pool = NULL;

return -1;

}

if (_options.thread_local_data_factory) {

_tl_options.thread_local_data_factory = _options.thread_local_data_factory;

if (bthread_key_create2(&_tl_options.tls_key, DestroyServerTLS,

_options.thread_local_data_factory) != 0) {

LOG(ERROR) << "Fail to create thread-local key";

return -1;

}

if (_options.reserved_thread_local_data) {

bthread_keytable_pool_reserve(_keytable_pool,

_options.reserved_thread_local_data,

_tl_options.tls_key,

CreateServerTLS,

_options.thread_local_data_factory);

}

} else {

_tl_options = ThreadLocalOptions();

}

应该没法废弃bthread_keytable_pool_t ，因为Server已经用了bthread_keytable_pool_t了，并对外提供了与之相关的设置项。

改造bthread_keytable_pool_t，将free_keytables由全局改成TLS，这样mutex就可以废弃了。同时修改reserved_thread_local_data语义：每个TLS上预留的data数量。这样可行吗？

如果使用TLS的话，当server结束调用Join时，通过bthread_keytable_pool_destroy方法需要删除所有线程内保存的TLS free_keytables才行，感觉也不是很方便？

我的理解，目前的实现是一个server会绑定一个全局的bthread_keytable_pool_t，在server的声明周期内，server和其生成的bthread使用同一个pool，在server结束时会销毁所有创建的KeyTable，通过bthread_keytable_pool_t内的链表；然后会从全局的KeyInfo中删除所有注册的Key。但是使用全局一个链表会存在锁的竞争问题，不知道理解的对不对。

MJY-HUST · 2024-05-20T18:34:20Z

改造bthread_keytable_pool_t ,其中引入一个类似Object_pool的成员，通过在成员内部使用tls变量来避免加互斥锁。然后对bthread_keytable_pool_t 中的锁的使用从互斥锁改为读写锁，来维护destroyed的值。在return_keytable内使用读锁，bthread_keytable_pool_init，bthread_keytable_pool_destroy时加写锁。这样就可以避免在sever未退出时多个线程return_keytable产生的锁的竞争。
不知道这样是否可行

chenBright · 2024-05-21T02:42:21Z

我的理解，目前的实现是一个server会绑定一个全局的bthread_keytable_pool_t，在server的声明周期内，server和其生成的bthread使用同一个pool，在server结束时会销毁所有创建的KeyTable，通过bthread_keytable_pool_t内的链表；然后会从全局的KeyInfo中删除所有注册的Key。但是使用全局一个链表会存在锁的竞争问题，不知道理解的对不对。

嗯，我也是这样理解的。不过一般接口实现的瓶颈都是在业务逻辑，所以这个锁的影响应该不大。

chenBright · 2024-05-21T03:04:07Z

改造bthread_keytable_pool_t ,其中引入一个类似Object_pool的成员，通过在成员内部使用tls变量来避免加互斥锁。然后对bthread_keytable_pool_t 中的锁的使用从互斥锁改为读写锁，来维护destroyed的值。在return_keytable内使用读锁，bthread_keytable_pool_init，bthread_keytable_pool_destroy时加写锁。这样就可以避免在sever未退出时多个线程return_keytable产生的锁的竞争。不知道这样是否可行

之前考虑过在bthread_keytable_pool_t中使用ObjectPool，但是ObjectPool好像不能实现reserved_thread_local_data吧？

destroyed应该可以用原子变量，就不用锁了吧。

chenBright · 2024-05-21T03:16:53Z

如果使用TLS的话，当server结束调用Join时，通过bthread_keytable_pool_destroy方法需要删除所有线程内保存的TLS free_keytables才行，感觉也不是很方便？

用butil::ThreadLocal应该挺方便的吧。
bthread_keytable_pool_init的时候，new一个butil::ThreadLocal。
bthread_keytable_pool_destroy的时候，delete。

MJY-HUST · 2024-05-21T03:34:37Z

如果使用TLS的话，当server结束调用Join时，通过bthread_keytable_pool_destroy方法需要删除所有线程内保存的TLS free_keytables才行，感觉也不是很方便？

用butil::ThreadLocal应该挺方便的吧。 bthread_keytable_pool_init的时候，new一个butil::ThreadLocal。 bthread_keytable_pool_destroy的时候，delete。

使用butil::ThreadLocal的话,new的object只有当线程结束才会释放，而目前基于bthread_keytable_pool_t的实现是在server Join后即清理释放所有的table，从KeyInfo中移除注册的key信息。如果使用butil::ThreadLocal，可以在delete后，析构所有的keytable吗？

MJY-HUST · 2024-05-21T03:40:49Z

改造bthread_keytable_pool_t ,其中引入一个类似Object_pool的成员，通过在成员内部使用tls变量来避免加互斥锁。然后对bthread_keytable_pool_t 中的锁的使用从互斥锁改为读写锁，来维护destroyed的值。在return_keytable内使用读锁，bthread_keytable_pool_init，bthread_keytable_pool_destroy时加写锁。这样就可以避免在sever未退出时多个线程return_keytable产生的锁的竞争。不知道这样是否可行

之前考虑过在bthread_keytable_pool_t中使用ObjectPool，但是ObjectPool好像不能实现reserved_thread_local_data吧？

destroyed应该可以用原子变量，就不用锁了吧。

应该也不能直接使用Object_pool，因为它是一个单例，不能做到sever间的隔离。
我看ResourcePool获取对象的模式是如下图，那么reserve时初始化多个全局的block是否可行。
return_keytable时，加读锁的临界区是判断destroyed + 将对象返回给pool，感觉不能直接使用原子变量

MJY-HUST · 2024-05-21T03:48:14Z

我的理解，目前的实现是一个server会绑定一个全局的bthread_keytable_pool_t，在server的声明周期内，server和其生成的bthread使用同一个pool，在server结束时会销毁所有创建的KeyTable，通过bthread_keytable_pool_t内的链表；然后会从全局的KeyInfo中删除所有注册的Key。但是使用全局一个链表会存在锁的竞争问题，不知道理解的对不对。

嗯，我也是这样理解的。不过一般接口实现的瓶颈都是在业务逻辑，所以这个锁的影响应该不大。

我们这边的场景是使用brpc server下面接了个rocksdb，当数据全被cache住时，service端的处理时间可能很短，导致瓶颈体现在了锁的竞争上

chenBright · 2024-05-21T03:57:52Z

如果使用TLS的话，当server结束调用Join时，通过bthread_keytable_pool_destroy方法需要删除所有线程内保存的TLS free_keytables才行，感觉也不是很方便？

用butil::ThreadLocal应该挺方便的吧。 bthread_keytable_pool_init的时候，new一个butil::ThreadLocal。 bthread_keytable_pool_destroy的时候，delete。

使用butil::ThreadLocal的话,new的object只有当线程结束才会释放，而目前基于bthread_keytable_pool_t的实现是在server Join后即清理释放所有的table，从KeyInfo中移除注册的key信息。如果使用butil::ThreadLocal，可以在delete后，析构所有的keytable吗？

butil::ThreadLocal析构时，会delete使用过butil::ThreadLocal的每个线程上的local data。应该可以满足需求吧。

chenBright · 2024-05-21T04:13:01Z

我看ResourcePool获取对象的模式是如下图，那么reserve时初始化多个全局的block是否可行。

ResourcePool也是单例哦。

return_keytable时，加读锁的临界区是判断destroyed + 将对象返回给pool，感觉不能直接使用原子变量

嗯嗯，return_keytable和bthread_keytable_pool_destroy需要互斥，不能用原子变量。

无论是ObjectPool还是ResourcePool的实现方式，即使destroyed=1，也是要将KeyTable返回给ObjectPool或者ResourcePool的吧。

MJY-HUST · 2024-05-21T04:54:30Z

如果使用TLS的话，当server结束调用Join时，通过bthread_keytable_pool_destroy方法需要删除所有线程内保存的TLS free_keytables才行，感觉也不是很方便？

用butil::ThreadLocal应该挺方便的吧。 bthread_keytable_pool_init的时候，new一个butil::ThreadLocal。 bthread_keytable_pool_destroy的时候，delete。

使用butil::ThreadLocal的话,new的object只有当线程结束才会释放，而目前基于bthread_keytable_pool_t的实现是在server Join后即清理释放所有的table，从KeyInfo中移除注册的key信息。如果使用butil::ThreadLocal，可以在delete后，析构所有的keytable吗？

butil::ThreadLocal析构时，会delete使用过butil::ThreadLocal的每个线程上的local data。应该可以满足需求吧。

请问一下这块逻辑具体在哪里呢？我没有看到butil::ThreadLocal这个类的定义。是ThreadLocalPointer吗？
当在一个线程上调用delete时，它是如何做到会delete使用过butil::ThreadLocal的每个线程上的local data，我没有看到类似链表或者list来存放所有线程的tls指针

chenBright · 2024-05-21T06:10:47Z

brpc/src/butil/thread_key.h

Lines 144 to 154 in b601c89

 template <typename T> 

 ThreadLocal<T>::~ThreadLocal() { 

 thread_key_delete(_key); 

 if (!_delete_on_thread_exit) { 

 BAIDU_SCOPED_LOCK(_mutex); 

 for (auto ptr : ptrs) { 

 DefaultDtor(ptr); 

 } 

 } 

 pthread_mutex_destroy(&_mutex); 

 }

MJY-HUST · 2024-05-23T02:43:25Z

@chenBright hello，我根据讨论修改了实现，可以帮忙看一下么

src/bthread/key.cpp

src/bthread/types.h

src/bthread/key.cpp

test/bthread_key_unittest.cpp

chenBright

LGTM

chenBright · 2024-05-30T02:17:18Z

@wwbmmm 也看看这个PR

wwbmmm · 2024-06-10T06:27:46Z

src/bthread/key.cpp

+ g->current_task()->local_storage.keytable = old_kt;
+ }
+ }
+ KeyTable* keytable;


class 类型不建议直接把成员暴露成public，要么改成struct，要么封装成getter/setter

改成了struct

wwbmmm · 2024-06-10T06:35:24Z

src/bthread/key.cpp

 delete kt;
 return;
 }
- kt->next = (KeyTable*)pool->free_keytables;
- pool->free_keytables = kt;
+ auto list = (butil::ThreadLocal<bthread::KeyTableList>*)pool->list;


全局free_keytables里的kt一旦被借走就永远还不回去free_keytables了吗？这样keytables的复用率会降低吧，如果线程数很多，可能会占用更多的内存

是不会返回给free_keytables了。free_keytables的大小是根据server.option的reserved_thread_local_data变量设置的，默认为0。
不过这里的线程指的应该只是一个bthread_keytable_pool_t pool所对应的server的brpc_worker threads，而不是普通的pthread。
所以相比使用单链表管理，如果brpc_worker thread是公平调度的话，内部的butil::ThreadLocal维护的链表长度应该是比使用一个单链表来说要短，但是性能会更好，因为同样存在回收机制，内存不会多太多。

wwbmmm · 2024-06-10T06:44:28Z

对这个场景有点疑问，如果bthread生命周期很短，为什么还要使用bthread_local变量呢，用pthread local、局部变量或者通过函数参数传递不行吗

MJY-HUST · 2024-06-11T16:48:03Z

对这个场景有点疑问，如果bthread生命周期很短，为什么还要使用bthread_local变量呢，用pthread local、局部变量或者通过函数参数传递不行吗

场景是：使用brpc框架通信，server下面接着一个数据库。使用bthread pool用于处理数据库的查询请求，数据库内部的实现使用了bthread级别的锁和tls来避免阻塞线程。数据库内部需要使用一些thread_local变量（bthread_local）来保证其一致性并减少重复创建。
并不是所有的条件下bthread的生命周期都会很短，但是在一些场景下，如果数据都在缓存中，那么一个bthread的生命周期就会很短，此时单链表结构的keytable pool管理模式就会出现瓶颈。

wwbmmm · 2024-06-13T04:26:15Z

LGTM

update

b339a7b

MJY-HUST force-pushed the keytable_optimize branch from d147ced to b339a7b Compare May 22, 2024 18:02

MJY-HUST changed the title ~~Use object_pool to store keytable by gflag~~ Use butil::ThreadLocal to store keytable May 23, 2024

chenBright reviewed May 23, 2024

View reviewed changes

update by comments

c707aa3

chenBright reviewed May 27, 2024

View reviewed changes

src/bthread/key.cpp Outdated Show resolved Hide resolved

src/bthread/key.cpp Outdated Show resolved Hide resolved

src/bthread/key.cpp Show resolved Hide resolved

src/bthread/key.cpp Outdated Show resolved Hide resolved

MJY-HUST added 3 commits May 27, 2024 20:42

update

ddb253f

fix

0fb0655

update test for using_pool

d279324

chenBright reviewed May 29, 2024

View reviewed changes

test/bthread_key_unittest.cpp Outdated Show resolved Hide resolved

fix format

c2bc88b

chenBright approved these changes May 30, 2024

View reviewed changes

format

98edccf

wwbmmm reviewed Jun 10, 2024

View reviewed changes

Change KeyTableList from class to struct

efcaba7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use butil::ThreadLocal to store keytable #2645

Use butil::ThreadLocal to store keytable #2645

MJY-HUST commented May 19, 2024 •

edited

chenBright commented May 20, 2024

chenBright commented May 20, 2024

MJY-HUST commented May 20, 2024 •

edited

MJY-HUST commented May 20, 2024 •

edited

chenBright commented May 21, 2024 •

edited

chenBright commented May 21, 2024 •

edited

chenBright commented May 21, 2024

MJY-HUST commented May 21, 2024

MJY-HUST commented May 21, 2024

MJY-HUST commented May 21, 2024

chenBright commented May 21, 2024

chenBright commented May 21, 2024

MJY-HUST commented May 21, 2024

chenBright commented May 21, 2024

MJY-HUST commented May 23, 2024

chenBright left a comment

chenBright commented May 30, 2024

wwbmmm Jun 10, 2024

MJY-HUST Jun 11, 2024

wwbmmm Jun 10, 2024

MJY-HUST Jun 11, 2024

wwbmmm commented Jun 10, 2024

MJY-HUST commented Jun 11, 2024 •

edited

wwbmmm commented Jun 13, 2024

Use butil::ThreadLocal to store keytable #2645

Are you sure you want to change the base?

Use butil::ThreadLocal to store keytable #2645

Conversation

MJY-HUST commented May 19, 2024 • edited

What problem does this PR solve?

What is changed and the side effects?

Check List:

chenBright commented May 20, 2024

chenBright commented May 20, 2024

MJY-HUST commented May 20, 2024 • edited

MJY-HUST commented May 20, 2024 • edited

chenBright commented May 21, 2024 • edited

chenBright commented May 21, 2024 • edited

chenBright commented May 21, 2024

MJY-HUST commented May 21, 2024

MJY-HUST commented May 21, 2024

MJY-HUST commented May 21, 2024

chenBright commented May 21, 2024

chenBright commented May 21, 2024

MJY-HUST commented May 21, 2024

chenBright commented May 21, 2024

MJY-HUST commented May 23, 2024

chenBright left a comment

Choose a reason for hiding this comment

chenBright commented May 30, 2024

wwbmmm Jun 10, 2024

Choose a reason for hiding this comment

MJY-HUST Jun 11, 2024

Choose a reason for hiding this comment

wwbmmm Jun 10, 2024

Choose a reason for hiding this comment

MJY-HUST Jun 11, 2024

Choose a reason for hiding this comment

wwbmmm commented Jun 10, 2024

MJY-HUST commented Jun 11, 2024 • edited

wwbmmm commented Jun 13, 2024

MJY-HUST commented May 19, 2024 •

edited

MJY-HUST commented May 20, 2024 •

edited

MJY-HUST commented May 20, 2024 •

edited

chenBright commented May 21, 2024 •

edited

chenBright commented May 21, 2024 •

edited

MJY-HUST commented Jun 11, 2024 •

edited