Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defragger improvements around large bins #12996

Merged
merged 15 commits into from
Feb 20, 2024

Conversation

sundb
Copy link
Collaborator

@sundb sundb commented Jan 26, 2024

Implement #12963

Changes

  1. large bins don't have external fragmentation or are at least non-defraggable, so we should ignore the effect of
    large bins when measuring fragmentation, and only measure fragmentation of small bins. this affects both the allocator_frag* metrics and also the active-defrag trigger
  2. Adding INFO metrics for allocator_muzzy memory, which is memory returned to the OS but still shows as RSS until the OS reclaims it.
  3. Adding a similar allocator.muzzy to MEMORY STATS

@sundb
Copy link
Collaborator Author

sundb commented Feb 2, 2024

@oranagra The implementation is now already going in the new direction(#12963 (comment)), and the different formula results in a lower fragmentation percent now than before, so the defrag test fails.

EDIT: fotget *100 to calculate percentage.

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.

p.s. i edited the top comment a bit since it seemed outdated.
PTAL

src/object.c Outdated Show resolved Hide resolved
src/zmalloc.c Show resolved Hide resolved
src/defrag.c Outdated Show resolved Hide resolved
src/zmalloc.c Show resolved Hide resolved
@oranagra oranagra linked an issue Feb 3, 2024 that may be closed by this pull request
src/object.c Outdated Show resolved Hide resolved
src/zmalloc.c Outdated Show resolved Hide resolved
src/defrag.c Outdated Show resolved Hide resolved
src/object.c Show resolved Hide resolved
sundb and others added 2 commits February 5, 2024 10:47
@sundb sundb marked this pull request as ready for review February 5, 2024 03:49
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.

@redis/core-team please approve

src/defrag.c Outdated Show resolved Hide resolved
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged labels Feb 5, 2024
src/zmalloc.c Show resolved Hide resolved
@madolson madolson added the state:needs-doc-pr requires a PR to redis-doc repository label Feb 19, 2024
@madolson
Copy link
Contributor

Adding INFO metrics for muzzy memory, which is memory returned to the OS but still shows as RSS until the OS reclaims it.

What is the point of showing muzzy to end users? Maybe after merging this we can improve the documentation around memory. It's something that people ask me about at AWS, since it's confusing.

@sundb
Copy link
Collaborator Author

sundb commented Feb 20, 2024

@madolson AFAIK, muzzy is very similar to retained, but muzzy is no longer reused but retained is.

@oranagra
Copy link
Member

AFAIK, muzzy is very similar to retained, but muzzy is no longer reused but retained is.

from my understanding that's wrong.

  • retained isn't part of RSS, only part of VMSIZE (which no one should care about).
  • muzzy is temporarily part of RSS (looks alarming), but the OS can reclaim that on it's own when it needs to (so we're not really holding that memory).

we should surely improve documentation.
the question here is if we wanna keep the jemalloc terminology, or try to create one of our own that we think could be clearer.
i have a feeling that creating new terminology can increase confusion.

@oranagra
Copy link
Member

approved in a core-team meeting.
@sundb can you create a redis-doc PR?

@oranagra oranagra merged commit f6785df into redis:unstable Feb 20, 2024
13 checks passed
@sundb sundb deleted the defrag_around_large branch February 21, 2024 01:29
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Feb 21, 2024
This field was added in redis#12996 but forgot to add it in json file.
This also causes reply-schemas-validator to fail.
@sundb
Copy link
Collaborator Author

sundb commented Feb 21, 2024

Redis-doc PR: redis/redis-doc#2672

oranagra pushed a commit that referenced this pull request Feb 21, 2024
This field was added in #12996 but forgot to add it in json file.
This also causes reply-schemas-validator to fail.
zuiderkwast pushed a commit to redis/redis-doc that referenced this pull request Mar 11, 2024
In redis/redis#12996 we add a new `allocator_muzzy` field for `INFO MEMORY` and `MEMORY STATS`.
Also update MEMORY-STATS.md for missing fields.
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:needs-doc-pr requires a PR to redis-doc repository
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

defragger improvements around large bins
5 participants