Skip to content

Try to trim strings only when applicable#11817

Merged
oranagra merged 6 commits intoredis:unstablefrom
uriyage:conditional-string-trimming
Feb 28, 2023
Merged

Try to trim strings only when applicable#11817
oranagra merged 6 commits intoredis:unstablefrom
uriyage:conditional-string-trimming

Conversation

@uriyage
Copy link
Contributor

@uriyage uriyage commented Feb 19, 2023

As sdsRemoveFreeSpace have 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.

  • 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.

REDISMODULE_NOT_USED(argc);
RedisModuleString *s = RedisModule_CreateString(ctx,"foo",3);
char *tmp = RedisModule_Alloc(1024);
RedisModule_StringAppendBuffer(ctx,s,tmp,1024);
Copy link
Contributor

@enjoy-binbin enjoy-binbin Mar 3, 2023

Choose a reason for hiding this comment

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

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.

https://github.com/redis/redis/actions/runs/4298577682/jobs/7492838351#step:6:249

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?

Copy link
Member

Choose a reason for hiding this comment

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

we probably need to skip that test when the allocator is not jemalloc

Copy link
Collaborator

Choose a reason for hiding this comment

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

let me change it in #11878?

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
@oranagra
Copy link
Member

oranagra commented Jul 2, 2023

anyone remembers why this PR isn't marked for backport, and #11878 is?
i see that the discussion in #11508 (comment) intended to backport it, so i wonder if we decided something else, or just forgot (if we did, then how come we didn't for 11878)?

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants