For future reference: https://blog.twitter.com/engineering/en_us/topics/infrastructure/2019/improving-key-expiration-in-redis.html

Hello @tejom, thanks for the PR! I think I'll end merging this in Redis 5 & unstable even if unstable is going to get a completely different implementation of expire soon, because Redis 5 will likely be used as the main Redis system for another 1/2 years in many shops. Before merging, given that this will hit a production release, I want to check why there was this obscure slowdown reported in the blog during the development. I'll ping you shortly.

Hey @antirez sounds good. I didn't miss anything from you did I?

@tejom Hi again! In the blog post you say that commenting the conditional made no difference because you use only one database. However I want to understand if you have Redis explicitly configured to have just one database, that is, what is the value of server.dbnum in your instances? Note that even if you use only one database, but still you have a default of 16 configured, the value of such global will be 16. Please may you clarify if you just use 1 DB or of you really have Redis set to only allow 1 DB? Thanks!

@antirez Oh I see. Yeah it had the default configured of 16. I wish I wrote that section better now, and there wasn't such a long time between working on this, writing and finally getting to publish it.

I guess still this ends up not doing anything in this test? The variable never changes. It would be one db used and 16 configured by default.

if (dbs_per_call > server.dbnum || timelimit_exit)
        dbs_per_call = server.dbnum;

would just be this and skip over it every time

if (16 > 16 || timelimit_exit)
        dbs_per_call = server.dbnum;

@antirez Any update on when this might get accepted?

Hello @madolson, I'm waiting to have a bit of time to check this PR because while I think it is a great addition, I'm not sure it should be exposed to the user in such terms of iterations / divisor, that are implementation details.

Hi @antirez. Checking in to see if you had considered this PR any further? My team is facing an issue almost exactly like what the Twitter team detailed in their blog. I'm very hopeful that either this or the radix tree based implementation you made reference to for Redis 6 could make a big improvement for us.

My own two cents - Exposing this seems very similar to maxmemory-samples. I could see a concern in exposing direct division as an implementation detail. Perhaps instead of making the divisor itself configurable, it could instead be like a "target %" config? For example, instead of using a config value of 4 to represent that it should continue if more than 25%, 25 itself could be the config value? The divisor would then be something like server.expired_threshold_divisor = atoi(argv[1]) / 100

This seems to be a slightly different approach than what was finally merged here: 84b01f6 (I couldn't find a relevant issue/pr).
See:

Right, IIRC this, and other discussions with Twitter is what lead Salvatore to implement the changes in 84b01f6.
I do hope these changes solved your concerns (and that you where able to converge to vanilla redis code and dismiss this private patch).

For the record, the big difference between the two approaches, is that this PR exposes tunables of the internal implementation in redis to the config, something that creates coupling with the internals, and means it'll be harder to later change.
What Salvatore implemented only exposes a generic active-expire-effort (0..10), which can in theory be applied on any other mechanism we'll want to implement.

Closing this one, feel free to post more details or open another PR if the latest code in unstable isn't satisfactory.