Try to trim strings only when applicable#11817
Merged
oranagra merged 6 commits intoredis:unstablefrom Feb 28, 2023
Merged
Conversation
oranagra
reviewed
Feb 19, 2023
sundb
reviewed
Feb 19, 2023
Co-authored-by: sundb <[email protected]>
oranagra
reviewed
Feb 20, 2023
oranagra
approved these changes
Feb 20, 2023
…ck for CLIENT_MODULE
oranagra
approved these changes
Feb 28, 2023
enjoy-binbin
reviewed
Mar 3, 2023
| REDISMODULE_NOT_USED(argc); | ||
| RedisModuleString *s = RedisModule_CreateString(ctx,"foo",3); | ||
| char *tmp = RedisModule_Alloc(1024); | ||
| RedisModule_StringAppendBuffer(ctx,s,tmp,1024); |
Contributor
There was a problem hiding this comment.
48834:M 01 Mar 2023 00:30:32.025 # <test> Testing test.string.trim
48834:M 01 Mar 2023 00:30:32.025 # <test> Test error reply: String was not trimmed as expected.
This will fail on MacOS, and it will also fail locally (my machine).
Looks like realloc doesn't shrink memory in this cas.
I tested that setting the number to 1300 can pass the test (local machine and daily).
Any other better idea? or reasons?
Member
There was a problem hiding this comment.
we probably need to skip that test when the allocator is not jemalloc
oranagra
pushed a commit
that referenced
this pull request
Mar 7, 2023
…11878) Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator. The reason is that malloc(33000) will allocate 65536 bytes(>42000). This test still passes under ubuntu with libc mem_allocator. ``` *** [err]: trim on SET with big value in tests/unit/type/string.tcl Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test) ``` simple test under mac m1 with libc mem_allocator: ```c void *p = zmalloc(33000); printf("malloc size: %zu\n", zmalloc_size(p)); # output malloc size: 65536 ```
oranagra
added a commit
that referenced
this pull request
May 30, 2023
This is a followup fix for #11817
Member
|
anyone remembers why this PR isn't marked for backport, and #11878 is? |
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
As `sdsRemoveFreeSpace` have an impact on performance even if it is a no-op (see details at redis#11508). Only call the function when there is a possibility that the string contains free space. * For strings coming from the network, it's only if they're bigger than PROTO_MBULK_BIG_ARG * For strings coming from scripts, it's only if they're smaller than LUA_CMD_OBJCACHE_MAX_LEN * For strings coming from modules, it could be anything. 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
…edis#11878) Test `trim on SET with big value` (introduced from redis#11817) fails under mac m1 with libc mem_allocator. The reason is that malloc(33000) will allocate 65536 bytes(>42000). This test still passes under ubuntu with libc mem_allocator. ``` *** [err]: trim on SET with big value in tests/unit/type/string.tcl Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test) ``` simple test under mac m1 with libc mem_allocator: ```c void *p = zmalloc(33000); printf("malloc size: %zu\n", zmalloc_size(p)); # output malloc size: 65536 ```
oranagra
pushed a commit
that referenced
this pull request
Sep 6, 2023
…11878) Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator. The reason is that malloc(33000) will allocate 65536 bytes(>42000). This test still passes under ubuntu with libc mem_allocator. ``` *** [err]: trim on SET with big value in tests/unit/type/string.tcl Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test) ``` simple test under mac m1 with libc mem_allocator: ```c void *p = zmalloc(33000); printf("malloc size: %zu\n", zmalloc_size(p)); # output malloc size: 65536 ``` (cherry picked from commit 3fba3cc)
oranagra
pushed a commit
that referenced
this pull request
Oct 18, 2023
…11878) Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator. The reason is that malloc(33000) will allocate 65536 bytes(>42000). This test still passes under ubuntu with libc mem_allocator. ``` *** [err]: trim on SET with big value in tests/unit/type/string.tcl Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test) ``` simple test under mac m1 with libc mem_allocator: ```c void *p = zmalloc(33000); printf("malloc size: %zu\n", zmalloc_size(p)); # output malloc size: 65536 ``` (cherry picked from commit 3fba3cc) (cherry picked from commit 646069a)
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.
As
sdsRemoveFreeSpacehave an impact on performance even if it is a no-op (see details at #11508).Only call the function when there is a possibility that the string contains free space.