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

Replace system allocator with mimalloc #3087

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Conversation

MartinquaXD
Copy link
Contributor

Description

I looked more into the supposed memory leak and noticed that the system allocator requires unreasonable amounts of memory compared to mimalloc so I replaced it for all the binaries.

Here we can see the memory footprint of the api pod with mimalloc (green) and the system allocator (yellow). The served traffic is the same but the system allocator continues to require more and more memory - probably because it's worse at dealing with memory fragmentation than mimalloc.

Screenshot 2024-10-26 at 17 10 42

@MartinquaXD MartinquaXD requested a review from a team as a code owner October 26, 2024 22:10
@MartinquaXD MartinquaXD force-pushed the replace-system-allocator branch from 5bace75 to a7959d5 Compare October 26, 2024 22:27
Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

Nice. When I was testing this initially, it didn't help at all since the memory growth wasn't so massive. Basically, this brings us back to the slow memory leak. Interesting, what exactly has increased the memory consumption lately.

@MartinquaXD
Copy link
Contributor Author

Interesting, what exactly has increased the memory consumption lately.

The first time this crazy memory usage happened was after merging #2996 but that does not contain any changes relevant to the API pod. :/
And for prod the issues started with release https://github.com/cowprotocol/services/releases/tag/v2.279.0 which also doesn't contain any significant code changes for the api pod. That release also merged the new deployement code but I don't expect that to have any influence either.

The only reasonable explanation that currently comes to mind is that the system memory allocator might have gotten replaced but I couldn't find good information on that. 🤷‍♂️

@MartinquaXD MartinquaXD merged commit c0b1f9e into main Oct 28, 2024
11 checks passed
@MartinquaXD MartinquaXD deleted the replace-system-allocator branch October 28, 2024 09:51
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
@mstrug
Copy link
Contributor

mstrug commented Oct 28, 2024

@MartinquaXD did you considered also jemalloc or tcmalloc?
There is some benchmarks that shows both of them are using less memory than mimalloc:
https://lf-hyperledger.atlassian.net/wiki/spaces/BESU/pages/22156632/Reduce+Memory+usage+by+choosing+a+different+low+level+allocator
Also other project was checking if replacing currently used jemalloc by mimalloc will gain performance to their project and most of the benchmark were worse: ClickHouse/ClickHouse#59497
Probably this depends on the particular project, so it would be good to have some benchmarks on our code which we can compare on different allocator.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants