Modules callbacks for lazy free effort, and unlink by chenyang8094 · Pull Request #7912 · redis/redis

chenyang8094 · 2020-10-15T03:44:24Z

Currently, 'dbAsyncDelete' does not specifically deal with the module data type, so the memory allocated in the module can only be freed synchronously. This is unfriendly for modules that will allocate a large amount of memory. In extreme cases, synchronous release of memory is easy to cause RT jitter. Therefore, this PR is to make the memory allocated by the module data type also have the opportunity to be freed asynchronously.

This PR adds an optional API to the RedisModuleTypeMethods structure, which is free_effort (its type is RedisModuleTypeFreeEffortFunc), which indicates the effort required to free a module memory. Currently, if the effort exceeds LAZYFREE_THRESHOLD, the module memory It may be released asynchronously.

At the same time, this PR also added a member'lazyfreed_objects' to INFO Memory (behind the 'lazyfree_pending_objects'
member), which represents the number of objects that have been lazyfree since redis was started, and 'lazyfree_pending_objects' indicates the pressure of the current memory asynchronous free queue. I think the indicator 'lazyfreed_objects' is still very useful in certain situations. For example, using the 'lazyfreed_objects' information in the TCL test corresponding to this PR can confirm that the memory of this module is freed asynchronously, rather than simply judging the memory drop as in 'lazyfree.tcl' . There is also the lazyfree mechanism that uses 'bio_mutex' to synchronize with the BIO thread, which will have some lock overhead, so we usually don’t want too many objects to release memory through lazyfree, and we can provide us with monitoring the growth trend of 'lazyfreed_objects' Good reference information, and if LAZYFREE_THRESHOLD becomes a configurable parameter in the future, the growth rate of 'lazyfreed_objects' can also be controlled by adjusting the value of LAZYFREE_THRESHOLD.

yossigo

Thank you @chenyang8094 for this PR!

src/redismodule.h

yossigo · 2020-10-18T12:43:23Z

tests/unit/moduleapi/test_lazyfree.tcl

I think this test might be flaky working at a byte level. I would consider allocating more substantial amount of memory in the test module (e.g. 500KB per entry or so) so it the effect would be more observable and robust.

Yes, I directly refer to'lazyfree.tcl' and change the memory allocation amount to 100000, which has far exceeded LAZYFREE_THRESHOLD. At the same time, because of the'lazyfreed_objects' information, I can be sure that the memory of this module is lazyfreed

soloestoy · 2020-10-20T12:23:58Z

It's a very useful feature and already worked in our product env. I think is's safe to merge after @yossigo suggestion : )

BTW, this PR contains another change, record counts of object freed in bio, info reply modification needs a major decision too.

yossigo · 2020-10-22T10:22:44Z

@soloestoy Thanks for the clarification!
@chenyang8094 chenyang8094 Please consider updating the PR description to more clearly indicate it includes both aspects (API change and INFO addition).

chenyang8094 · 2020-10-23T02:43:13Z

@soloestoy Thanks for the clarification!
@chenyang8094 chenyang8094 Please consider updating the PR description to more clearly indicate it includes both aspects (API change and INFO addition).

Ok, I have updated it

oranagra · 2020-10-23T14:13:45Z

I would like to use this opportunity to add a few more changes in that area.

First, in the spirit of #7865, it would be nice if a module can get the compiled version of REDISMODULE_TYPE_METHOD_VERSION so that when registering the callbacks it knows which of them are gonna be used and which will be ignored.

Secondly, I know a few complicated modules also keep some global information about their keys (outside redis's dict), while this is a bit of a violation, they get away with it since redis is not able to do anything with that pointer without the module's assistance (not even free it), but now that redis can detach that key from the keyspace in the main thread, and free it in a background thread, i think we need to give the module a little bit more help.

What i think would solve it is to add another detach callback which will also carry the keyname being detached. this will solve two things.

In some say it covers for the missing keyname argument from the free callback (i know some modules really miss it).
It tells the module that this pointer is no longer part of the database, and also that it will be soon freed by a thread.

I would like to get some feedback from @MeirShpilraien, @swilly22, @guybe7.
If we agree on the details, can ask @chenyang8094 to extend this PR, or one of us can make a followup PR, but we probably don't wanna release that without these (and more?) additions (due to the version bump).

P.S. i vaguely remember there were a few other things we wanna do by iterating on module data structure (inside its value pointer), which would be nice to add soon too, but i don't recall what.
i.e. we already have mem_usage and digest, we're missing a defrag and i think something else, but i don't recall.
maybe CLONE support for #6599?

MeirShpilraien · 2020-10-23T21:10:13Z

@oranagra I agree we need the detach callback so we can also detach the object from globals and just free the memory on the free callback. I am not sure if the detach and free_effort can somehow combine, do we use the free_effort on more places other then free the key?

oranagra · 2020-10-24T05:15:57Z

@MeirShpilraien i meant that the detach callback should be called before a normal free too.
This way the detach is always called when detaching a value, from the db, and the free callback is just to release the memory.
Also, it mans you kinda have the key name for the non-lazy free.
P.s it might be a good idea to add an attach callback too, so on rdbload you don't assume it was added to the db. This way we can some day do the deserialization in a thread, and the attach in the main thread, and modules that do funny things will do them in the attach and wont suffer from multi threading issues.

MeirShpilraien · 2020-10-24T18:24:53Z

@oranagra I get the idea of the detach, I was just wondering if it makes sense to combine the detach and the free_effort to make the API less verbose (for example, maybe the detach can return the free effort, and then Redis can choose if he wants to free the memory immediately or pass it to a background thread). Not sure if it's a good idea, just a thought, what do you think?

Regarding the attach, today we have the loaded keyspace notification for modules only, maybe we can somehow combine it there? Again to make the API less verbose?

oranagra · 2020-10-25T07:52:15Z

@MeirShpilraien i'm not sure about combining free_effort and detach. usually redis knows if it's willing to consider doing a lazy free or not, so combining these will force the module to "compute" the free effort even when there's no need.
on the other hand, this computation is meant to be quick (and doesn't have to be accurate).

regarding loaded keyspace notification, and attach, i don't think these two are related.
the keyspace notification is received for all keys types, and the attach would happen only for the module type.
in addition the attach can carry the actual module data pointer with it (possibly saving the module an RM_OpenKey).

@danni-m FYI - i think you also had trouble with free not providing the key name.

yossigo · 2020-10-26T07:29:18Z

@oranagra I think all of this calls for a more careful design, and we don't necessarily have to address it all at once or as part of this PR.

One issue is exposing the two-step lazy free to modules, so they also get an unlink callback. This makes sense to me as a direct extension to this PR. It will require considering what is the expected outcome depending on what callbacks are/aren't implemented, and how this affects backwards compatibility.

The issue of the opposite link callback seems to me unrelated, and I'm not even sure it represents a problem at present time. Note that rdbload already does not imply the key gets linked (e.g. with RM_LoadDataTypeFromString()) and this is indicated by not having a key name associated with the RedisModuleIO context.

oranagra · 2020-10-26T11:54:28Z

@yossigo i agree much of what i raised is not directly related and can be handled separately (just felt that this is an opportunity to raise it).

The detach / unlink thing though is a bit related, at least in the sense that we should avoid releasing the API in this PR until we finished designing that part too since we might wanna change that API.

p.s. even if the additional API doesn't change the API of this PR, i rather not increment REDISMODULE_TYPE_METHOD_VERSION twice.

regarding backwards compatibility, if the detach and free effort come in the same version, then i see no problem, old modules that don't implement, will always be freed from the main thread.

So what do you suggest? wanna call the core-team to approve it and merge it as is, and then open another PR to add / modify it?
Or try to at least integrate the detach / unlink callback into this PR?

chenyang8094 · 2020-10-28T02:33:16Z

@yossigo i agree much of what i raised is not directly related and can be handled separately (just felt that this is an opportunity to raise it).

The detach / unlink thing though is a bit related, at least in the sense that we should avoid releasing the API in this PR until we finished designing that part too since we might wanna change that API.

p.s. even if the additional API doesn't change the API of this PR, i rather not increment REDISMODULE_TYPE_METHOD_VERSION twice.

regarding backwards compatibility, if the detach and free effort come in the same version, then i see no problem, old modules that don't implement, will always be freed from the main thread.

So what do you suggest? wanna call the core-team to approve it and merge it as is, and then open another PR to add / modify it?
Or try to at least integrate the detach / unlink callback into this PR?

I can understand what you mean very much. detach mainly solves the problem that free callback does not provide parameter key. When I was writing a module product, I did save the key separately in the index of the module, but it was really difficult for me to get this information when the key was deleted. I think detach is useful for free/lazyfree. If possible, I can extend this PR and add detach and free_effort to the v3 version.

oranagra · 2020-10-28T06:49:29Z

@chenyang8094 i think that we all agree that unlink (better fits redis terminology) makes sense, and probably is a must-have feature in order to push the lazy free feature in.
We need to make sure this can work in a backwards compatible manner for old modules that are not aware of it, but i actually don't see any problem (seems it'll work good already with the trivial implementation).
Please go ahead and add this change to this PR, it'll be easier to discuss when we see the code.

chenyang8094 · 2020-10-29T06:30:46Z

@chenyang8094 i think that we all agree that unlink (better fits redis terminology) makes sense, and probably is a must-have feature in order to push the lazy free feature in.
We need to make sure this can work in a backwards compatible manner for old modules that are not aware of it, but i actually don't see any problem (seems it'll work good already with the trivial implementation).
Please go ahead and add this change to this PR, it'll be easier to discuss when we see the code.

@yossigo @soloestoy @oranagra @MeirShpilraien I have tried to modify the code according to my own understanding, please review it again, thank you.

oranagra · 2020-10-29T14:31:57Z

src/module.c

maybe we want to let the module have access to the mt (val) too?
some modules may want to look into some variable they have there, and be forced to do RM_OpenKey for that.

another alternative is maybe pass a RedisModuleKey and then they can get the key name via RM_GetKeyNameFromModuleKey, and value via RM_ModuleTypeGetValue

what do you guys thing?

Well, If necessary, I'd prefer to just pass val over because it's already there.

@guybe7 @MeirShpilraien @yossigo what do you think?
i suppose a void* value is consistent with other APIs:

typedef void (*RedisModuleTypeDigestFunc)(RedisModuleDigest *digest, void *value); typedef void (*RedisModuleTypeUnlinkFunc)(RedisModuleString *key, const void *value);

i'm ok with this, but i want to give it a bit more thought so that we don't regret it in the future.

The alternative of providing a RedisModuleKey* will let the user get the key name, value and also other things (like LRU) from it.

@oranagra I lean towards just passing the value, I don't see any real need to manipulate the key (after all it's getting unlinked) and it's more consistent with the other calls.

src/module.c

src/redismodule.h

tests/modules/test_lazyfree.c

oranagra

i'm ok with the current API.
let's wait a day to see if others raise some objections.

src/module.c

tests/unit/moduleapi/test_lazyfree.tcl

oranagra · 2020-11-02T15:58:58Z

@chenyang8094 thank you.
one more thing i need to ask: we need a better commit comment that describes the changes.
one way is to edit / update the top comment in this PR, which i can use when squash-merging the PR (will create one commit in unstable).
Or if we want to keep it two separate commits, you need to edit the commit comments and force-push.

@redis/core-team please approve:

new RM_GetTypeMethodVersion
new module type callback for lazy free effort
new module type callback for detaching a key from the database prior to freeing it (comes with key name and value)
new lazyfreed_objects info field

chenyang8094 · 2020-11-03T12:11:33Z

@chenyang8094 thank you.
one more thing i need to ask: we need a better commit comment that describes the changes.
one way is to edit / update the top comment in this PR, which i can use when squash-merging the PR (will create one commit in unstable).
Or if we want to keep it two separate commits, you need to edit the commit comments and force-push.

@redis/core-team please approve:

new RM_GetTypeMethodVersion

new module type callback for lazy free effort

new module type callback for detaching a key from the database prior to freeing it (comes with key name and value)

new lazyfreed_objects info field

I have used rebase to merge the two commits into one, and re-edited the commit message. Please check again if there are any problems, thank you.

chenyang8094 · 2020-11-08T11:20:17Z

@chenyang8094 maybe i'm missing the point of your last post, but i think what @guybe7 meant is that in case of error, he'd like to log the key name (on both unlink and free_effort callbacks).

p.s. regarding your previous post, we currently call the free_effort before unlink (not after it).
        robj *val = dictGetVal(de);
        size_t free_effort = lazyfreeGetFreeEffort(val);	        size_t free_effort = lazyfreeGetFreeEffort(val);


        /* Tells the module that the key has been unlinked from the database */
        moduleNotifyKeyUnlink(key,val);
we can probably change that, but i don't like modules to rely on the order of these.

at first i thought that modules should not include the global data in their estimated free_effort, since they must release that global data in the unlink (not in the free callback, since it can be called form a thread), but maybe some module would like to detach data from a global structure and attach it to the object, and that the freeing of that data will be done from the thread.

in such case the effort of the unlink remains small, and the one in free becomes bigger.

since we can't predict what some crazy modules will do or need, i think we should have the key in both callbacks. and i think we can probably change the order of these calls (just makes a little bit more sense).

@chenyang8094 i don't understand what you mean by "This will lead to several linkage modifications".

Yes, the free_effort order is indeed wrong, I have modified it. I have added the parameter key to free_effort, which also led to the modification of the freeObjAsync function. Please review it, thank you.

oranagra · 2020-11-08T13:34:20Z

src/db.c

i now realize that we're missing a call to moduleNotifyKeyUnlink here (better be done before calling dictSetVal).

which makes me wonder what else are we missing?

madolson · 2020-11-09T23:57:42Z

src/module.c

I see this is a direct translation from OS, but I think the notion of free_effort is unnecessary and confusing from a module context. I feel like a better callback would be, can_free_async(), which can make it's own decision about whether or not this is worth freeing async.

@madolson what do you mean by "OS"?
we're trying to match the effort to:

#define LAZYFREE_THRESHOLD 64

i think a name like can_free_async is misleading (it's possible that it "can" but better not do that). if we're seeking a boolean return, maybe a better name would be should_free_async.
but then if we some day let users control this constant via command or config, the modules will need to be able to query it.
i think returning the complexity (how many pointers you have to free, or how many loops you'll need to run) makes sense as a return value.
i suppose we better document that 0 indicate not to be released by a thread.

Open source? I didn't know what to call none module redis code, redis core maybe?

I still think should free async makes a more sense. The lazy free estimate is an approximation for "should we free this async". A module doesn't need to make the same decision we do.

I also think we should never expose the lazy free effort as a parameter, without some type of conversion in between. You could be more explicit and pass the effort to the module callback function and it can choose if it wants to ignore it. I just think we should have more thoughtful apis. From an outside user it's very hard to understand.

yossigo

@chenyang8094 Thank you not just for this PR, but also for your patience with this process! Getting to an agreement on names and API interfaces is always a challenge (and don't get me wrong on this, it is for all the right reasons).

I didn't get a chance to voice my opinion before. I totally agree with passing the key to unlink, but I don't think it's really necessary for free_effort (although not feeling strongly against it).

@madolson I think you have a valid argument here, but I think that however we turn it the API will still end up with extra coupling. Even if we expose LAZYFREE_THRESHOLD we still have this rigid contract that will be hard to break, and if we just let modules decide we lose all control. So, as none of the options is ideal I'd stick with this one which is simple. We do have to document it properly though.

oranagra · 2020-11-10T20:34:19Z

so in my eyes, only thing that's left is a call to moduleNotifyKeyUnlink in dbOverwrite
@chenyang8094 can you take care of this?

madolson · 2020-11-11T01:34:56Z

@yossigo i'll buy the simplicity argument. I do think we should say returning 0 should be explicitly documented that it calls the async free function every time. Seem reasonable?

chenyang8094 · 2020-11-11T01:35:21Z

so in my eyes, only thing that's left is a call to moduleNotifyKeyUnlink in dbOverwrite
@chenyang8094 can you take care of this?

@oranagra @yossigo Yes, I understand very well, it is very necessary to do things right. @oranagra @yossigo Thanks for your review, I also thought that moduleNotifyKeyUnlink also needs to be called during flushall/flushdb, right? Since I have been busy with the Double 11 these past two days, I will update PR again after this. Thank you for your suggestions and patience.

madolson

API lgtm, I did not look at the code much.

oranagra · 2020-11-11T15:48:57Z

@chenyang8094 good point about FLUSHDB, and more importantly FLUSHDB ASYNC.
but i don't think we can afford to run on the entire keyspace when detaching the dict from the database, just to call the module unlink callback.

@MeirShpilraien WDYT? maybe it should be documented that when the module gets the flushdb event hook it should expect the unexpected?

MeirShpilraien · 2020-11-11T15:58:03Z

@oranagra so on flushbd async we will not get the unlink callback and module should use the flush event to handle such case? LGTM but we need to make sure to document it.

oranagra · 2020-11-11T16:31:35Z

@chenyang8094 trying to sum up what's left (64 comments, LOL):

call moduleNotifyKeyUnlink in dbOverwrite (better be done before calling dictSetVal)
when free_effort returns 0, always do an async free (consider that an infinite return value)
document the above.
document in the unlink callback, that it won't be called on flushdb (both sync and async), and the module can use the RedisModuleEvent_FlushDB to hook into that.

Add two optional callbacks to the RedisModuleTypeMethods structure, which is `free_effort` and `unlink`. the `free_effort` callback indicates the effort required to free a module memory. Currently, if the effort exceeds LAZYFREE_THRESHOLD, the module memory may be released asynchronously. the `unlink` callback indicates the key has been removed from the DB by redis, and may soon be freed by a background thread. Add `lazyfreed_objects` info field, which represents the number of objects that have been lazyfreed since redis was started. Add `RM_GetTypeMethodVersion` API, which return the current redis-server runtime value of `REDISMODULE_TYPE_METHOD_VERSION`. You can use that when calling `RM_CreateDataType` to know which fields of RedisModuleTypeMethods are gonna be supported and which will be ignored.

itamarhaber

Haven't reviewed but...

oranagra · 2020-11-16T08:34:58Z

merged.
@chenyang8094 thank you for your patience and dedication.

Add two optional callbacks to the RedisModuleTypeMethods structure, which is `free_effort` and `unlink`. the `free_effort` callback indicates the effort required to free a module memory. Currently, if the effort exceeds LAZYFREE_THRESHOLD, the module memory may be released asynchronously. the `unlink` callback indicates the key has been removed from the DB by redis, and may soon be freed by a background thread. Add `lazyfreed_objects` info field, which represents the number of objects that have been lazyfreed since redis was started. Add `RM_GetTypeMethodVersion` API, which return the current redis-server runtime value of `REDISMODULE_TYPE_METHOD_VERSION`. You can use that when calling `RM_CreateDataType` to know which fields of RedisModuleTypeMethods are gonna be supported and which will be ignored.

yossigo requested changes Oct 18, 2020

View reviewed changes

yossigo added the state:major-decision Requires core team consensus label Oct 18, 2020

chenyang8094 requested a review from yossigo October 22, 2020 09:13

yossigo previously approved these changes Oct 22, 2020

View reviewed changes

chenyang8094 requested a review from yossigo October 23, 2020 02:13

chenyang8094 closed this Oct 28, 2020

chenyang8094 reopened this Oct 28, 2020

chenyang8094 dismissed yossigo’s stale review via 1d79dcf October 29, 2020 06:22

oranagra reviewed Oct 29, 2020

View reviewed changes

chenyang8094 requested a review from oranagra October 30, 2020 02:44

oranagra reviewed Nov 1, 2020

View reviewed changes

tests/modules/test_lazyfree.c Outdated Show resolved Hide resolved

oranagra reviewed Nov 1, 2020

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

tests/unit/moduleapi/test_lazyfree.tcl Outdated Show resolved Hide resolved

chenyang8094 requested a review from oranagra November 2, 2020 15:23

oranagra previously approved these changes Nov 2, 2020

View reviewed changes

chenyang8094 dismissed oranagra’s stale review via 7bede24 November 3, 2020 12:07

oranagra reviewed Nov 8, 2020

View reviewed changes

madolson reviewed Nov 9, 2020

View reviewed changes

yossigo previously approved these changes Nov 10, 2020

View reviewed changes

madolson previously approved these changes Nov 11, 2020

View reviewed changes

chenyang8094 dismissed stale reviews from madolson and yossigo via 76f7f01 November 12, 2020 06:21

Merge branch 'unstable' into unstable

494ba0f

chenyang8094 requested review from madolson, oranagra, soloestoy and yossigo November 12, 2020 06:30

oranagra approved these changes Nov 12, 2020

View reviewed changes

soloestoy approved these changes Nov 13, 2020

View reviewed changes

itamarhaber approved these changes Nov 15, 2020

View reviewed changes

yossigo approved these changes Nov 15, 2020

View reviewed changes

oranagra changed the title ~~Modules allocated memory can be reclaimed in the background~~ Modules callbacks for lazy free effort, and unlink Nov 16, 2020

oranagra merged commit c1aaad0 into redis:unstable Nov 16, 2020

oranagra mentioned this pull request Jan 13, 2021

Redis 6.2 RC1. #8187

Merged

enjoy-binbin mentioned this pull request Jan 5, 2022

Add lazyfreed_objects field in INFO redis/redis-doc#1731

Merged

JimB123 mentioned this pull request Sep 14, 2022

Enhance mem_usage/free_effort/unlink/copy callbacks and add GetDbFromIO Api. #8999

Merged

Conversation

chenyang8094 commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soloestoy commented Oct 20, 2020

Uh oh!

yossigo commented Oct 22, 2020

Uh oh!

chenyang8094 commented Oct 23, 2020

Uh oh!

oranagra commented Oct 23, 2020

Uh oh!

MeirShpilraien commented Oct 23, 2020

Uh oh!

oranagra commented Oct 24, 2020

Uh oh!

MeirShpilraien commented Oct 24, 2020

Uh oh!

oranagra commented Oct 25, 2020

Uh oh!

yossigo commented Oct 26, 2020

Uh oh!

oranagra commented Oct 26, 2020

Uh oh!

chenyang8094 commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Oct 28, 2020

Uh oh!

chenyang8094 commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Nov 2, 2020

Uh oh!

chenyang8094 commented Nov 3, 2020

Uh oh!

chenyang8094 commented Nov 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Nov 10, 2020

Uh oh!

madolson commented Nov 11, 2020

Uh oh!

chenyang8094 commented Nov 11, 2020

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

chenyang8094 commented Oct 15, 2020 •

edited

Loading

chenyang8094 commented Oct 28, 2020 •

edited

Loading