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

Setup jemalloc in default Dockerfile to optimize memory allocation #50943

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

northeastprince
Copy link
Contributor

Ruby's use of malloc can create memory fragmentation problems, especially when using multiple threads like Puma does. Switching to an allocator that uses different patterns to avoid fragmentation can decrease memory usage by a substantial margin.

Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

I agree we should do this, as it's really a no brainer in term of performance.

However as is this PR break a few stuff and need some more work. IIRC the jemalloc package comes with a command that does the proper LD_PRELOAD, we should just use that.

@byroot byroot merged commit 0add5db into rails:main Feb 4, 2024
3 of 4 checks passed
@northeastprince northeastprince deleted the jemalloc branch February 4, 2024 17:20
@gjtorikian
Copy link
Contributor

gjtorikian commented Feb 5, 2024

Apologies for the drive-by comment, but I think the "better way" would be to patch Ruby directly to use libjemalloc, as mastodon shows: https://github.com/mastodon/mastodon/blob/1666b1955992e16f4605b414c6563ca25b3a3f18/Dockerfile#L108-L113

LD_PRELOAD affects every binary, not just Ruby, which may not be desirable. More details here.

Happy to make a PR for this change if it is desired!

@byroot
Copy link
Member

byroot commented Feb 5, 2024

I'm aware of this technique, but I don't want to patch a binary that comes from a previous layer, and the LD_PRELOAD leaking isn't a big concern except for Google Chrome, which is an extremely niche use in production, and is easily worked around by add ENV.delete("LD_PRELOAD") in config/boot.rb or something.

@gjtorikian
Copy link
Contributor

Gotchu!

@@ -1,5 +1,10 @@
#!/bin/bash -e

# Enable jemalloc for reduced memory usage and reduce latency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo:

Suggested change
# Enable jemalloc for reduced memory usage and reduce latency.
# Enable jemalloc for reduced memory usage and reduced latency.

Or just removing is more succint imo:

Suggested change
# Enable jemalloc for reduced memory usage and reduce latency.
# Enable jemalloc for reduced memory usage and latency.

Copy link
Member

Choose a reason for hiding this comment

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

@suwyn
Copy link

suwyn commented Feb 9, 2024

@byroot @northeastprince I'm not seeing jemalloc being used in the process map. Can you double check?

Compare the output of

pmap -X 1 | grep jemalloc

on the rails server process vs the output using

exec env LD_PRELOAD="$LD_PRELOAD" "${@}"

in the entrypoint script

PikachuEXE added a commit to PikaSer-Cosmos/likecoin-chain-tx-indexer-pika that referenced this pull request Feb 12, 2024
byroot added a commit that referenced this pull request Feb 12, 2024
Otherwise it won't applied to the execed process.

Fix: #50943 (comment)
@byroot
Copy link
Member

byroot commented Feb 12, 2024

Yeah, I forgot to add export. Fixed in 9940dc8. Thanks for noticing.

Ridhwana pushed a commit to Ridhwana/rails that referenced this pull request Mar 7, 2024
Otherwise it won't applied to the execed process.

Fix: rails#50943 (comment)
viralpraxis pushed a commit to viralpraxis/rails that referenced this pull request Mar 24, 2024
Otherwise it won't applied to the execed process.

Fix: rails#50943 (comment)
fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024
Otherwise it won't applied to the execed process.

Fix: rails#50943 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants