Skip to content

Conversation

@tezc
Copy link
Collaborator

@tezc tezc commented Nov 21, 2024

Starting from #13133, we allocate a jemalloc thread cache and use it for lua vm.
On certain cases, like script flush or function flush command, we free the existing thread cache and create a new one.

Though, for function flush, we were not actually destroying the existing thread cache itself. Each call creates a new thread cache on jemalloc and we leak the previous thread cache instances. Jemalloc allows maximum 4096 thread cache instances. If we reach this limit, Redis prints "Failed creating the lua jemalloc tcache" log and abort.

There are other cases that can cause this memory leak, including replication scenarios when emptyData() is called.

The implication is that it looks like redis used_memory is low, but allocator_allocated and RSS remain high.

Co-authored-by: debing.sun <debing.sun@redis.com>
@tezc tezc requested review from oranagra and sundb November 21, 2024 05:57
@CLAassistant
Copy link

CLAassistant commented Nov 21, 2024

CLA assistant check
All committers have signed the CLA.

@tezc tezc added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 21, 2024
unsigned int lua_tcache = (unsigned int)(uintptr_t)ud;
#endif

lua_close(lua);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved down this call not to access its ud field after lua_close().

@tezc tezc merged commit 9ebf80a into redis:unstable Nov 21, 2024
14 checks passed
@tezc tezc deleted the fix-func-leak branch November 21, 2024 11:13
@YaacovHazan YaacovHazan moved this from Todo to Pending in Redis 7.4 Backport Apr 21, 2025
@YaacovHazan YaacovHazan moved this from Pending to Todo in Redis 7.4 Backport Apr 21, 2025
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request Apr 22, 2025
…3661)

Starting from redis#13133, we allocate a
jemalloc thread cache and use it for lua vm.
On certain cases, like `script flush` or `function flush` command, we
free the existing thread cache and create a new one.

Though, for `function flush`, we were not actually destroying the
existing thread cache itself. Each call creates a new thread cache on
jemalloc and we leak the previous thread cache instances. Jemalloc
allows maximum 4096 thread cache instances. If we reach this limit,
Redis prints "Failed creating the lua jemalloc tcache" log and abort.

There are other cases that can cause this memory leak, including
replication scenarios when emptyData() is called.

The implication is that it looks like redis `used_memory` is low, but
`allocator_allocated` and RSS remain high.

Co-authored-by: debing.sun <debing.sun@redis.com>
@YaacovHazan YaacovHazan moved this from Todo to Done in Redis 7.4 Backport Apr 29, 2025
@sundb sundb added this to Redis 8.0 Aug 15, 2025
@sundb sundb removed this from Redis 8.2 Aug 15, 2025
@sundb sundb moved this from Todo to Done in Redis 8.0 Aug 15, 2025
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
…3661)

Starting from redis#13133, we allocate a
jemalloc thread cache and use it for lua vm.
On certain cases, like `script flush` or `function flush` command, we
free the existing thread cache and create a new one.

Though, for `function flush`, we were not actually destroying the
existing thread cache itself. Each call creates a new thread cache on
jemalloc and we leak the previous thread cache instances. Jemalloc
allows maximum 4096 thread cache instances. If we reach this limit,
Redis prints "Failed creating the lua jemalloc tcache" log and abort.

There are other cases that can cause this memory leak, including
replication scenarios when emptyData() is called.

The implication is that it looks like redis `used_memory` is low, but
`allocator_allocated` and RSS remain high.

Co-authored-by: debing.sun <debing.sun@redis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants