refactor: move tl_cluster_config into cluster_config.cc #4714
Conversation
BorysTheDev
force-pushed
the
place_Tl_cluster_config_in_ClusterConfig
branch
2 times, most recently
from
ec504bc to
72697ab
Compare
BorysTheDev
force-pushed
the
place_Tl_cluster_config_in_ClusterConfig
branch
from
72697ab to
9bd1d77
Compare
| @@ -17,6 +17,9 @@ using namespace std; | |||
| namespace dfly::cluster { | |||
|
|
|||
| namespace { | |||
|
|
|||
| thread_local shared_ptr<ClusterConfig> tl_cluster_config; | |||
There was a problem hiding this comment.
I don't understand. Why is this a shared_ptr when it's a thread local ? Why do you need atomic ref count on a non concurrent (thread local) storage ?
There was a problem hiding this comment.
Oh I just realized this was moved nvm. General remark, I still think shared pointer for a single thread is an antipattern + you do atomic operations on data that is not shared among threads.
There was a problem hiding this comment.
we share the same config between all threads
There was a problem hiding this comment.
Then why is it a thread local ?
There was a problem hiding this comment.
And also, each call to current now is a copy and an atomic increment. If this is used concurrently we might stall the processors (especially on NUMA). We should really use the threadlocal when we are in scope and we know that the object won't be destructed (we did that previously)
There was a problem hiding this comment.
discussed offline
|
|
||
| auto config = tl_cluster_config->GetConfig(); | ||
| auto config = ClusterConfig::Current()->GetConfig(); |
There was a problem hiding this comment.
Before, you used the thread local directly. Now you do instead: copy of the shared_ptr which increments atomically the control block/ref count and then call GetConfig(). Is there a reason for this ?
There was a problem hiding this comment.
If you don't need ref count and can gurantee the lifetime, why not simply return a plain pointer ?
There was a problem hiding this comment.
We don't know when the life of the config will be finished. So it is the reason of shared_ptr.
There was a problem hiding this comment.
That's fine but why then is it a thread local ?
yes. |
if you need cluster_config you don't need cluster_family now