acl check api for functions and eval#10220
Merged
oranagra merged 8 commits intoredis:unstablefrom Feb 7, 2022
Merged
Conversation
- Misc cleanups. - Remove optimizations done in 48c49c4 and 4f68655: unclear why this was added. Commit message claims a 4% performance increase which I couldn't recreate and might not be worth it even if it did recreate. Updated code is easier to re-user and much simpler. Details of the benchmarks I did are in the PR (redis#10091).
Contributor
Author
|
@MeirShpilraien I'd like you to look specifically and the old optimization code I removed in this PR and verify my logic makes sense. Please read the top comment. |
oranagra
reviewed
Feb 3, 2022
Member
oranagra
left a comment
There was a problem hiding this comment.
mainly reviewed the interfaces.
leaving the Lua binding code for Meir to review.
Member
|
@redis/core-team please approve |
oranagra
approved these changes
Feb 3, 2022
Member
|
@yoav-steinberg can you please make a redis-doc PR? |
Member
|
ohh, you already did.. sorry. |
Merged
Member
|
for the record, i tested this specific commit (on unstable at the time it was merged) and (taken from the top comment of this one) both showed around 5% performance degradation (same as what we see in #11541 when adding it back on top of recent unstable) |
oranagra
added a commit
that referenced
this pull request
Dec 5, 2022
…7.0 (#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in #10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <[email protected]> Co-authored-by: sundb <[email protected]>
oranagra
added a commit
that referenced
this pull request
Dec 12, 2022
…7.0 (#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in #10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <[email protected]> Co-authored-by: sundb <[email protected]> (cherry picked from commit 2d80cd7)
madolson
pushed a commit
to madolson/redis
that referenced
this pull request
Apr 19, 2023
…7.0 (redis#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in redis#10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <[email protected]> Co-authored-by: sundb <[email protected]>
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
…7.0 (redis#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in redis#10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <[email protected]> Co-authored-by: sundb <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See issue #10091.
This PR:
redis.acl_check_cmd()api to lua scripts. It can be used to check if the current user has permissions to execute a given command. The new function receives the command to check as an argument exactly likeredis.call()receives the command to execute as an argument.redis.acl_check_cmd()API and theredis.[p]call()API. This cleans up potential duplicate code.argvarray in theredis.[p]call()implementation. These optimizations were introduced years ago in 48c49c4 and 4f68655. It is unclear why this was added. The original commit message claims a 4% performance increase which I couldn't recreate and might not be worth it even if it did recreate. This PR removes that optimization. Following are details of the benchmark I did that couldn't reveal any performance improvements due to this optimization:I ran the benchmark on this branch with and without commit 68b7168
Results in requests per second:
As you can see, different use cases showed identical or negligible performance differences. So finally I decided to chuck the original optimization and simplify the code.