Request for RCP #13

[antirez]

Key locks for modules "no GIL" threaded operations.

[yossigo]

Hi @antirez, I have read this only a couple of times and surely did not think of everything, so please consider this just preliminary feedback:

Blocking Commands: This specification relies a lot on them, and IMHO it makes a lot of sense to take this approach. However currently blocking operations are not "first class citizens" in Redis (i.e. limitations on Lua, RM_Call(), slaves, etc.) so there may be large hidden gaps in implementing it as it is.

Thread-Safe Key Access Patterns: The use cases for key access from a thread safe context are diverse. The obvious is a module blocking command that operates in the background on well known keys associated with the command, but we should remember other things like command implementation that depend on arbitrary key access, background module house-keeping, garbage collection, etc.

In such cases I guess it'll be inevitable for the GIL to still exist, if only to synchronize between the thread's attempt to obtain a key lock and any concurrent operation which may be taking place in the Redis main loop without a key lock.

Locking Mechanism: The locked_keys dict offers a clean decoupled way to handle locking, but I have some concerns about its performance impact if it becomes the normal path (i.e. no longer likely empty locked keys dict). Other alternatives would be to lock groups of keys, e.g. a hash bucket or part of the radix tree (when it replaces the global dict). In a massively concurrent system with lots of threads, loss of lock granularity can be painful. In our case I don't think we're looking at so much concurrency (if we did get to the point we have hundreds of threads accessing large parts of the keyspace, we have bigger design problems anyway).

Another thing we need to consider is how this affects the consistency model (both for clients and for replicated slaves). This may already be an issue with modules implementing thread safe contexts, but encouraging this and possibly moving parts of the Redis core in this direction will surely make this more significant.