Optimization: moduleLoadString should try to create embedded string#11050
Optimization: moduleLoadString should try to create embedded string#11050oranagra merged 1 commit intoredis:unstablefrom
Conversation
valentinogeron
force-pushed
the
fix-module-string-load
branch
from
5530dce to
f3511f1
Compare
| robj *o = encode ? tryCreateStringObject(SDS_NOINIT,len) : | ||
| tryCreateRawStringObject(SDS_NOINIT,len); |
There was a problem hiding this comment.
i see this condition was added here: c7822bf
before that it always used to create raw string.
i went looking for it in fear that this PR reverts an intentional change and there was some reason for it.
but i suppose it just just plain mistake.
i.e. at the time, that commit should have changed the code from always creating a raw string into something that automatically decides according to the size, but instead a condition on encode was added, which i think was a mistake.
|
@yossigo i'd like a second set of eyes on this one. |
oranagra
changed the title
|
@oranagra It looks correct to me, but potentially non-trivial implications if it uncovers other issues. I think it should be in the release notes at least for this reason. |
|
looking at it deeper, it affects anyone who called rdbGenericLoadStringObject with RDB_LOAD_NONE, since RDB_LOAD_SDS and RDB_LOAD_PLAIN don't go into that path, and RDB_LOAD_ENC was already ok. so other than the module implications, the others seem safe. |
oranagra
added
the
release-notes
|
actually, before i do, @MeirShpilraien please take a look too to consider any module implications |
|
Looks OK 👍 . Don't see any issue with it. |
…f not plain (redis#11050) Before this change, if the module has an embedded string, then uses RedisModule_SaveString and RedisModule_LoadString, the result would be a raw string instead of an embedded string. Now the `RDB_LOAD_ENC` flag to `moduleLoadString` only affects integer encoding, but not embedded strings (which still hold an sds in the robj ptr, so they're actually still raw strings for anyone who reads them). Co-authored-by: Valentino Geron <[email protected]>
4 participants
Before this change, if the module has an embedded string, then uses RedisModule_SaveString and RedisModule_LoadString, the result would be a raw string instead of an embedded string.
Now the
RDB_LOAD_ENCflag tomoduleLoadStringonly affects integer encoding, but not embedded strings (which still hold an sds in the robj ptr, so they're actually still raw strings for anyone who reads them).