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

Add jemalloc support #198

Closed
wants to merge 11 commits into from
Closed

Add jemalloc support #198

wants to merge 11 commits into from

Conversation

marcotc
Copy link

@marcotc marcotc commented Mar 9, 2018

Third time is the charm!
I had this PR open for a few weeks and realized there's work being done around jemalloc.

Should we add jemalloc only for Debian? If so, I'll just move my logic to the template file.

This PR adds jemalloc all Linux images: debian and slim.

I tried to make it generic, but I'm not sure we need this level of complexity here.

fixes #182, closes #190

@marcotc
Copy link
Author

marcotc commented Mar 9, 2018

My initial intention was to create a new set of images, to avoid any backwards incompatibility, if any:

* 2.5.0-jemalloc
* 2.5.0-stretch-jemalloc
* 2.5.0-slim-jemalloc
* 2.5.0-slim-stretch-jemalloc

But it seems like there are talks about just adding jemalloc the default Debian images.

One option is to only use jemalloc for the newest Ruby releases, keeping users of older versions intact.
This would prevent the creation of a lot of duplicate files, but reduce risks to an extent.

@@ -25,8 +25,11 @@ RUN set -ex \
libssl1.0-dev \
ruby \
' \
&& runtimeDeps=' \
libjemalloc-dev
' \
Copy link
Author

Choose a reason for hiding this comment

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

I had to add it as a separate set of dependencies, because buildDeps get purged after the build is finished.

@ianks
Copy link

ianks commented Mar 15, 2018

Is there a particular reason not to do this with alpine as well?

@marcotc
Copy link
Author

marcotc commented Mar 15, 2018

@ianks not particularly. I'll give at shot at building alpine images.

@marcotc
Copy link
Author

marcotc commented Mar 15, 2018

@ianks I just tried to compile Alpine with jemalloc and ended up getting stuck on this Ruby/Alpine/Jemalloc bug: https://bugs.ruby-lang.org/issues/13524

It seems like we are out of luck for the time being for Alpine+jemalloc.

@n-rodriguez
Copy link

n-rodriguez commented Apr 11, 2018

This is weird because as far as I know Redis uses jemalloc and there is a redis:alpine Docker image :/
https://github.com/sickp/docker-alpine-redis

@marcotc
Copy link
Author

marcotc commented Apr 12, 2018

@n-rodriguez It seems like this has to do with how Ruby and Redis handle the pointer returned by getcwd differently:

@n-rodriguez
Copy link

@marcotc thanks for the answer!

@@ -62,6 +66,9 @@ RUN set -ex \
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& rm -r /root/.gem/

# TODO remove me before merging
RUN ruby -r rbconfig -e "RbConfig::CONFIG['LIBS'].include?('jemalloc') ? puts('Ruby is compiled with jemalloc') : warn('JEMALLOC IS MISSING FROM RUBY')"
Copy link

Choose a reason for hiding this comment

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

maybe nice to leave this as a sanity check with RUN ruby -r rbconfig -e "abort 'jemalloc not enabled' unless RbConfig::CONFIG['LIBS'].include?('jemalloc')"

@callumj
Copy link

callumj commented Apr 21, 2018

Thanks for working on this @marcotc, we're currently "patching" jemalloc support off the official Docker library using some custom scripts so this is a huge help by getting it official.

@ababich
Copy link

ababich commented May 15, 2018

There is no issues in https://github.com/hoteltonight/ruby so may be I can comment here

As I understood this is published in Docker Hub as https://hub.docker.com/r/hoteltonight/ruby-jemalloc/ - is it possible to make relations between docker hub, github repo, etc. more transparent?

@cheerfulstoic
Copy link

I don't understand the issues that are going on, but it would be awesome to have jmalloc support built-in to our Ruby containers. Is this stalled?

@yosifkit
Copy link
Member

yosifkit commented Jul 3, 2018

I think we were still uncertain whether this is the recommended or endorsed way to build ruby according to ruby upstream. We try to keep the images as close to upstream release recommendations as possible.

See also #192 (comment).

@marcotc
Copy link
Author

marcotc commented Jul 4, 2018

Would a PR with a new separate set of debian images using jemalloc be acceptable?
I can make those changes if that's the case.

@cheerfulstoic
Copy link

Also, would it be possible to specify an environment variable when building? There seems to be a --with-jemalloc flag which could be used (though I imagine there would need to be at least one other conditional to install jemalloc beforehand

@shleeable
Copy link
Contributor

Hey @marcotc , #241 will hopefully fix the alpine blocker.

@marcotc
Copy link
Author

marcotc commented Oct 19, 2018

Jemalloc is not "sticking" to Ruby 2.6rc as it does with older versions.
I don't see the library linked anymore, even though we built with it:

Step 11/17 : RUN ruby -r rbconfig -e "puts RbConfig::CONFIG['LIBS']"
 ---> Running in e0dc1e0e4115
-lm

Still investigating this.

@marcotc
Copy link
Author

marcotc commented Oct 19, 2018

Now we have this scenario:
Jemalloc works fine for Ruby < 2.6 on Debian, and possibly Alpine 3.8, which would make the template file even more complex.

I'm wondering if we should just publish a separate set of images with the prefix -jemalloc, and not touch the "vanilla" images.

@tianon
Copy link
Member

tianon commented Oct 19, 2018

I wholeheartedly agree, and recommend that such work happen in a separate repo/image until Ruby upstream decides jemalloc is something they want to officially support and recommend for general use. 👍

@marcotc
Copy link
Author

marcotc commented Oct 21, 2018

Just to clarify @tianon, are you saying jemalloc images should not be hosted under https://hub.docker.com/_/ruby/, not even if labelled appropriately?

@tianon
Copy link
Member

tianon commented Oct 21, 2018 via email

@marcotc
Copy link
Author

marcotc commented Oct 21, 2018

I totally understand @tianon, thanks for clarifying.

@shleeable
Copy link
Contributor

Is there a forked image I can use? or are we not there yet?

@giovannibonetti
Copy link

Here in my company we have been using this fork from Hotel Tonight in production for a few months and it works great. We use hoteltonight/ruby-jemalloc:2.5.1-stretch

@ababich
Copy link

ababich commented Nov 30, 2018

@ashleyhull-versent I used hoteltonight image for some time but it is much easier to build your own based on official docker image, just take a look at 2-3 changes in Dockerfile and go ahead

@shleeable
Copy link
Contributor

Does anybody know who's actually at fault with this? Is there a ticket upstream with Ruby or Alpine?

@yosifkit yosifkit mentioned this pull request Jan 21, 2019
@yosifkit
Copy link
Member

Closing PR until a recommendation from Ruby upstream. Discussion can continue on #182.

See also #182 (comment)

@yosifkit yosifkit closed this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruby with jemalloc