chenBright commented on code in PR #2645:
URL: https://github.com/apache/brpc/pull/2645#discussion_r1612880616
##########
src/bthread/key.cpp:
##########
@@ -204,14 +205,54 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTable {
SubKeyTable* _subs[KEY_1STLEVEL_SIZE];
};
+class KeyTableList {
+public:
+ KeyTableList() {
+ keytable = NULL;
+ }
+ ~KeyTableList() {
+ bthread::TaskGroup* const g = bthread::tls_task_group;
+ bthread::KeyTable* old_kt = bthread::tls_bls.keytable;
+ while (keytable) {
+ bthread::KeyTable* kt = keytable;
+ keytable = kt->next;
+ bthread::tls_bls.keytable = kt;
+ if (g) {
+ g->current_task()->local_storage.keytable = kt;
+ }
Review Comment:
什么场景下，old_kt == kt呢？
##########
src/bthread/key.cpp:
##########
@@ -226,14 +267,15 @@ void return_keytable(bthread_keytable_pool_t* pool,
KeyTable* kt) {
delete kt;
return;
}
- std::unique_lock<pthread_mutex_t> mu(pool->mutex);
+ pthread_rwlock_rdlock(&pool->rwlock);
if (pool->destroyed) {
- mu.unlock();
+ pthread_rwlock_unlock(&pool->rwlock);
delete kt;
return;
}
- kt->next = (KeyTable*)pool->free_keytables;
- pool->free_keytables = kt;
+ kt->next = pool->list->get()->keytable;
+ pool->list->get()->keytable = kt;
+ pthread_rwlock_unlock(&pool->rwlock);
}
static void cleanup_pthread(void* arg) {
Review Comment:
嗯，就是纯pthread调用bthread代码这种情况。
##########
src/bthread/key.cpp:
##########
@@ -226,14 +267,15 @@ void return_keytable(bthread_keytable_pool_t* pool,
KeyTable* kt) {
delete kt;
return;
}
- std::unique_lock<pthread_mutex_t> mu(pool->mutex);
+ pthread_rwlock_rdlock(&pool->rwlock);
if (pool->destroyed) {
- mu.unlock();
+ pthread_rwlock_unlock(&pool->rwlock);
delete kt;
return;
}
- kt->next = (KeyTable*)pool->free_keytables;
- pool->free_keytables = kt;
+ kt->next = pool->list->get()->keytable;
+ pool->list->get()->keytable = kt;
+ pthread_rwlock_unlock(&pool->rwlock);
Review Comment:
想了一下，应该先不用加这个逻辑吧。放回全局链表，到时候再取，都要加写锁，还会跟操作tls
list有竞争，所以限制上线应该没必要。全局链表只保留预先reserve的功能就好了吧。
或者不放回全局链表，而是直接delete。
##########
src/bthread/key.cpp:
##########
@@ -204,14 +205,54 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTable {
SubKeyTable* _subs[KEY_1STLEVEL_SIZE];
};
+class KeyTableList {
+public:
+ KeyTableList() {
+ keytable = NULL;
+ }
+ ~KeyTableList() {
+ bthread::TaskGroup* const g = bthread::tls_task_group;
+ bthread::KeyTable* old_kt = bthread::tls_bls.keytable;
+ while (keytable) {
+ bthread::KeyTable* kt = keytable;
+ keytable = kt->next;
+ bthread::tls_bls.keytable = kt;
+ if (g) {
+ g->current_task()->local_storage.keytable = kt;
+ }
+ delete kt;
+ if (old_kt == kt) {
+ old_kt = NULL;
+ }
+ }
+ bthread::tls_bls.keytable = old_kt;
+ if(g) {
+ g->current_task()->local_storage.keytable = old_kt;
+ }
+ }
+ KeyTable* keytable;
+};
+
static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) {
- if (pool != NULL && pool->free_keytables) {
- BAIDU_SCOPED_LOCK(pool->mutex);
- KeyTable* p = (KeyTable*)pool->free_keytables;
+ if (pool != NULL && (pool->list->get()->keytable || pool->free_keytables))
{
+ pthread_rwlock_rdlock(&pool->rwlock);
+ KeyTable* p = pool->list->get()->keytable;
if (p) {
- pool->free_keytables = p->next;
+ pool->list->get()->keytable = p->next;
+ pthread_rwlock_unlock(&pool->rwlock);
return p;
}
+ pthread_rwlock_unlock(&pool->rwlock);
Review Comment:
这里加读锁对性能有影响吗？
##########
src/bthread/key.cpp:
##########
@@ -204,14 +205,54 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTable {
SubKeyTable* _subs[KEY_1STLEVEL_SIZE];
};
+class KeyTableList {
+public:
+ KeyTableList() {
+ keytable = NULL;
+ }
+ ~KeyTableList() {
+ bthread::TaskGroup* const g = bthread::tls_task_group;
+ bthread::KeyTable* old_kt = bthread::tls_bls.keytable;
+ while (keytable) {
+ bthread::KeyTable* kt = keytable;
+ keytable = kt->next;
+ bthread::tls_bls.keytable = kt;
+ if (g) {
+ g->current_task()->local_storage.keytable = kt;
+ }
+ delete kt;
+ if (old_kt == kt) {
+ old_kt = NULL;
+ }
+ }
+ bthread::tls_bls.keytable = old_kt;
+ if(g) {
+ g->current_task()->local_storage.keytable = old_kt;
+ }
+ }
+ KeyTable* keytable;
+};
+
static KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) {
- if (pool != NULL && pool->free_keytables) {
- BAIDU_SCOPED_LOCK(pool->mutex);
- KeyTable* p = (KeyTable*)pool->free_keytables;
+ if (pool != NULL && (pool->list->get()->keytable || pool->free_keytables))
{
+ pthread_rwlock_rdlock(&pool->rwlock);
+ KeyTable* p = pool->list->get()->keytable;
if (p) {
- pool->free_keytables = p->next;
+ pool->list->get()->keytable = p->next;
+ pthread_rwlock_unlock(&pool->rwlock);
return p;
}
+ pthread_rwlock_unlock(&pool->rwlock);
Review Comment:
参考类似bvar、DoublyBufferedData的实现，每个KeyTableList有一把锁和destroyed成员变量。
borrow_keytable：加tls
KeyTableList自己的锁，如果destroyed=0，取尝试从keytable链表取可用的KeyTable。否则直接new。
bthread_keytable_pool_destroy：不delete pool->list，而是利用
#2632 的特性，遍历所有tls KeyTableList，加锁，置destroyed=1，释放keytable链表上的KeyTable。
这样可行吗？但是这样有个问题：pool->list和pool->rwlock一样，不能析构。
##########
src/bthread/types.h:
##########
@@ -83,8 +83,17 @@ inline std::ostream& operator<<(std::ostream& os,
bthread_key_t key) {
}
#endif // __cplusplus
+namespace bthread{
+class KeyTableList;
+}
+
+namespace butil {
+template <typename T> class ThreadLocal;
+}
+
typedef struct {
- pthread_mutex_t mutex;
+ pthread_rwlock_t rwlock;
+ butil::ThreadLocal<bthread::KeyTableList>* list;
void* free_keytables;
Review Comment:
我的意思是`void* list`，跟free_keytables的处理一样。这里是C代码，不能用C++代码吧。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
