Call makeObjectShared to normalize a shared object into a true shared object #1566
Conversation
… object Currently, only int and shared.redacted call makeObjectShared for shared objects. Although this breaks the blame log, there is a few points i would like to mention: 1. For threading, our fork for some reasons calling decrRefCount in the threads for shared.command. Although this is not used in OSS code now, it may be a issue in the future. 2. For cleanup, all shared objects's refcount should be the same. 3. For CoW, probably can reduce CoW although it is very minor. Signed-off-by: Binbin <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1566 +/- ##
============================================
+ Coverage 70.78% 70.98% +0.19%
============================================
Files 120 120
Lines 65046 65071 +25
============================================
+ Hits 46045 46189 +144
+ Misses 19001 18882 -119
|
| @@ -1964,153 +1964,153 @@ void afterSleep(struct aeEventLoop *eventLoop, int numevents) { | |||
| * initializations can be moved back inside createSharedObjects() below. */ | |||
| void createSharedObjectsWithCompat(void) { | |||
| const char *name = server.extended_redis_compat ? "Redis" : SERVER_TITLE; | |||
| if (shared.loadingerr) decrRefCount(shared.loadingerr); | |||
There was a problem hiding this comment.
I'm struggling with this, not sure how to handle it yet.
There was a problem hiding this comment.
It's a little sad that we need to store two versions of these strings and need to check the compat flag every time we return these strings. We have already planned to remove this compat config at some time.
If we can make extended-redis-compat a readonly config, we can initialize them only once, but that's not perfect. One of the purposes is that users can change them in runtime if they have some problem.
Another idea: These strings are changed only when the compat config is changed. Is it possible to wait for all threads to stop and then replace the shared objects and free the old ones? We do something similar for MODULE UNLOAD to make sure no module is accessing a command that is going to be removed:
void moduleUnregisterCommands(struct ValkeyModule *module) {
/* Drain IO queue before modifying commands dictionary to prevent concurrent access while modifying it. */
drainIOThreadsQueue();WDYT?
There was a problem hiding this comment.
lazyfree job does not fit in the I/O threads, unforunately
Signed-off-by: Binbin <[email protected]>
Currently, only int and shared.redacted call makeObjectShared for
shared objects.
Although this breaks the blame log, there is a few points i would
like to mention:
the threads for shared.command. Although this is not used in OSS
code now, it may be a issue in the future.