Skip to content

acl check api for functions and eval#10220

Merged
oranagra merged 8 commits intoredis:unstablefrom
yoav-steinberg:functions_eval_acl_check
Feb 7, 2022
Merged

acl check api for functions and eval#10220
oranagra merged 8 commits intoredis:unstablefrom
yoav-steinberg:functions_eval_acl_check

Conversation

@yoav-steinberg
Copy link
Contributor

@yoav-steinberg yoav-steinberg commented Jan 31, 2022

See issue #10091.
This PR:

  1. Adds the 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 like redis.call() receives the command to execute as an argument.
  2. In the PR I unified the code used to convert lua arguments to redis argv arguments from both the new redis.acl_check_cmd() API and the redis.[p]call() API. This cleans up potential duplicate code.
  3. While doing the refactoring in 2 I noticed there's an optimization to reduce allocation calls when parsing lua arguments into an argv array in the redis.[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:
benchmark 1: src/redis-benchmark -P 500 -n 10000000 eval 'return redis.call("ping")' 0
benchmark 2: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__")' 0
benchmark 3: src/redis-benchmark -P 500 -r 1000 -n 100000 eval "for i=1,100,1 do redis.call('set','kk'..i,'vv'..__rand_int__) end return redis.call('get','kk5')" 0
benchmark 4: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")'

I ran the benchmark on this branch with and without commit 68b7168
Results in requests per second:

cmd without optimization without optimization 2nd run with original optimization with original optimization 2nd run
1 461233.34 477395.31 471098.16 469946.91
2 34774.14 35469.8 35149.38 34464.93
3 6390.59 6281.41 6146.28 6464.12
4 28005.71   27965.77  

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.

@yoav-steinberg yoav-steinberg changed the title wip: acl check api for functions and eval acl check api for functions and eval Jan 31, 2022
- 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).
@yoav-steinberg yoav-steinberg marked this pull request as ready for review February 3, 2022 14:01
@yoav-steinberg yoav-steinberg added the state:needs-doc-pr requires a PR to redis-doc repository label Feb 3, 2022
@yoav-steinberg
Copy link
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 oranagra linked an issue Feb 3, 2022 that may be closed by this pull request
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainly reviewed the interfaces.
leaving the Lua binding code for Meir to review.

@oranagra
Copy link
Member

oranagra commented Feb 3, 2022

@redis/core-team please approve

@oranagra oranagra added approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Feb 3, 2022
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (not full CR).

Copy link

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@oranagra
Copy link
Member

oranagra commented Feb 7, 2022

@yoav-steinberg can you please make a redis-doc PR?

@oranagra
Copy link
Member

oranagra commented Feb 7, 2022

ohh, you already did.. sorry.

@oranagra oranagra merged commit 9dfeda5 into redis:unstable Feb 7, 2022
@oranagra oranagra mentioned this pull request Feb 28, 2022
@oranagra
Copy link
Member

oranagra commented Dec 4, 2022

for the record, i tested this specific commit (on unstable at the time it was merged)
with both (taken from #11541)

memtier_benchmark --command="eval \"redis.call('hset', 'h1', 'k', 'v');redis.call('hset', 'h2', 'k', 'v');return redis.call('ping')\" 0"  - --hide-histogram --test-time 60 --pipeline 10 -x 1

and (taken from the top comment of this one)

src/redis-benchmark -P 500 -r 1000 -n 100000 eval "for i=1,100,1 do redis.call('set','kk'..i,'vv'..__rand_int__) end return redis.call('get','kk5')" 0

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Add api for functions / eval for ACL check

5 participants