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

Refactor for easier deploys of new versions. Adds jemalloc 5.0.1 #18

Closed
wants to merge 17 commits into from

Conversation

gaffneyc
Copy link
Collaborator

@gaffneyc gaffneyc commented Dec 11, 2017

This PR rewrites a large chunk of the build system and how the buildpack is installed and configured. The goals with this PR are to make using jemalloc easier for folks and to reduce the time and work required to release new versions.

The build system has been changed to build specific versions for multiple Heroku stacks against the official Heroku docker images. This means building a new version is a single command make VERSION=5.0.1 followed by uploading the build artifacts to a release on GitHub.

There has been some confusion lately around setting LD_PRELOAD especially since the revision changed with 5.0.0. This introduces JEMALLOC_ENABLED which can configure LD_PRELOAD automatically. This has the added benefit of being opt-in rather than opt-out. People have the option to either set this or use jemalloc.sh on a per-dyno basis.

Versions can now be switched by setting JEMALLOC_VERSION and pushing a new code change / recompiling the slug. The previous incarnation of versions required there to be a specific tag on GitHub with the version string. Now, anyone using the buildpack will always have the most up to date installer script and can switch between versions as long as there is a build available. This also means no longer needing to make code changes to release a new version of jemalloc.

There is some configuration that needs to happen if or when this PR is merged. It uses a different organization for releases where there is a tag based on name of the Heroku stack (heroku-16 and cedar-14). I'm not aware of a way to create a release on GitHub that only contains uploaded binaries (i.e. doesn't include any source) so I created a release-master that is an orphaned commit with no code on it. See the releases on my fork for a reference. I'm happy to configure the releases here to match it or be responsible for updating new builds on my fork going forward.

Finally, this work was sponsored by Dead Man's Snitch (a cron monitoring service), where we deployed 5.0.1 and have seen some really great results on both Puma and Sidekiq.

Fixes #2
Fixes #7
Fixes #13
Closes #11
Closes #14

mojodna and others added 17 commits June 15, 2017 14:46
The previous version added leading and trailing colons to these values if a variable had no assigned value before the buildpack script ran.
This allows new versions to be built for uploading by running
`make VERSION=5.0.1` rather than updating the hard coded version in
multiple places. I've also added support for building against either the
legacy cedar-14 architecture or heroku-16. While versions built on
cedar-14 appear to run on heroku-16, the newer stack should also mean a
newer and better tool chain.

I've removed the Dockerfile and switched to using Heroku's docker images
directly as this removes the need for cleaning them up afterwards. It
possibly makes debugging harder but having build.sh available should
avoid some of that.

Fixes #2
Fixes #7
Fixes #13
These should only be necessary if someone is planning to link against
jemalloc when building other libraries. I'm erring on the side that it's
probably unnecessary and I'm removing it to simplify the install
command.
It looks like the code was originally taken from a Cairo build pack that
had multiple libraries it was installing. Removing it reduces the
overall size and complexity of the script.
This is a quality of life addition that makes it easier to configure
LD_PRELOAD. It appears the library name changed between 4.x and 5.x
which looks to have caused some confusion. It also seems that getting it
in the Heroku dashboard acts differently from setting it on the command
line (I think).
Allows selecting a precompiled version of jemalloc given a version
string. This allows easily switching but requires a rebuild of the slug.
Previously all builds were against Cedar, which is probably fine, but
building against newer stacks may give benefits from a new toolchain.
This switches to a strategy of having a release we stack that contains
all of the specific version of jemalloc.

This also drops the additional build version number (e.g. 5.0.1-1) as
it's not configurable by the user and could be different per version.
Anyone using the build pack will always get the newest build of a
particular version that's available.
This reduces the artifact sizes by 20 to 25%.
It looks like the server for canonware.com has been decommissioned and
all releases are being posted to GitHub now.

https://gitter.im/jemalloc/jemalloc?at=59b07988bc46472974040c91
An issue came up that jemalloc 3.6.0 does not include jemalloc-config
(it was added in 4.0.0). Researching the issue I discovered that
installing jemalloc will create a symlink that points to the correct
revision. Using jemalloc-config is more correct especially in cases
where multiple versions of jemalloc are installed. Since Heroku dyno's
are built from a fresh state on each code change, we can expect that
there will only ever be one version of jemalloc installed and the
symlink will point to the latest version.
We've seen some great results with this so I'm trying to make getting
started with it easier and more understandable. This highlights the
configuration options that are available and adds a section for how to
deploy new releases (because I'm sure I'll forget in 6 months).
Heroku will be supporting cedar-14 until April 2019, it's cedar-10
that's officially be deprecated.

https://devcenter.heroku.com/articles/stack
@jonekdahl
Copy link

jonekdahl commented Dec 20, 2017

Having support for heroku-16 would be great as we had to remove the buildpack from our app when upgrading to heroku-16 recently.

@gaffneyc
Copy link
Collaborator Author

@jonekdahl take a look at my fork which is the basis for this PR. It's usable today and support heroku-16

@jonekdahl
Copy link

Any chance of this getting merged? I am still using @gaffneyc's fork and it works well for us (on heroku-16 with JEMALLOC_VERSION=3.6.0) but I would prefer to use the upstream repo.

@gaffneyc
Copy link
Collaborator Author

@jonekdahl I plan to keep my fork up to date and maintained since we've deployed it to a number of apps in the last month and switching back would be a lot of work.

@nateberkopec
Copy link
Collaborator

OK, I'm back in the open-source world.

@gaffneyc Anything that needs to be changed/updated here before I sit down and take a look?

@gaffneyc
Copy link
Collaborator Author

@nateberkopec No, this is good to go. It depends on a specific setup for the releases tab before being merged (which I mention in the description).

I'm planning to keep my fork maintained, if you or mojodna don't have capacity then I'm happy to take on ownership of this.

@mojodna
Copy link
Owner

mojodna commented Mar 28, 2018

@gaffneyc I don't have capacity, so thanks for volunteering! @nateberkopec has admin rights on this repo (pretty sure), so I'll defer to him on how to continue supporting this.

@jayelkaake
Copy link

I think this should be merged - worked great for us! Thank you sir!

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented May 9, 2018

For anyone following this, jemalloc 5.1.0 was released yesterday. I've added it as well as builds for the upcoming Heroku-18 stack to my fork. It looks like 5.1.0 is a recommended upgrade by the jemalloc developers and we've just deployed it to our staging environment. Hopefully if all goes well I'll make 5.1.0 the default in a month or so.

@nateberkopec
Copy link
Collaborator

LGTM, suggest adding @gaffneyc as maintainer and let him merge/release this.

@mojodna
Copy link
Owner

mojodna commented May 9, 2018

@gaffneyc I've just invited you as a collaborator. Thanks!!!

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented May 9, 2018

Awesome, thanks. I've done a bit of work on my fork since this PR. I'll see about getting this merged, releases put together, and other changes added in here.

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Jul 2, 2018

Sorry, I haven't had time to come back and get this merged. The problem is that it's a rebased and edited version of an old version of what's on master on my branch right now. I'll try to schedule some time this week to make it happen.

@czj
Copy link

czj commented Jul 19, 2018

Thanks a lot for your work @gaffneyc il would be awesome :)

@bf4
Copy link

bf4 commented Sep 2, 2018

@gaffneyc @nateberkopec I can't tell what's blocking this PR from being merged. Does it need to be broken up into smaller PRs? I'm happy to help, just don't want to get started without checking in.

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Sep 2, 2018

@bf4 This is blocked by me having time to do it. It's based off of my fork but it's both lagged behind master on my fork and was a modified rebase to keep the downloads coming from this repo. To get the PR up to date I would need to go through that process again from my master branch and add the releases. It's been hard to prioritize the time since my fork is working for what we need.

If you need what's in this PR today then I would say just use my fork. If you have time to help get it merged, going through the rebase process and opening a new PR would help a bunch.

@bf4
Copy link

bf4 commented Sep 3, 2018

If I understand what you mean, is that this PR is based on a different branch than what your'e using now, and that since the pr source branch, you've made changes to your master which should be part of any PR ? gaffneyc/heroku-buildpack-jemalloc@gaffneyc:upgrade-jemalloc...master Asking because the PR we're commenting on looked fine to me, though I admittedly haven't run it. I'll try cherry-picking some stuff.

@bf4
Copy link

bf4 commented Sep 3, 2018

@gaffneyc I took your master minus the sponsorship commit, since I figure that's a separate discussion, and merged this repos master into it. Was a clean merge. #19

@wongy91
Copy link

wongy91 commented Sep 13, 2018

how do i verify if jemalloc is running on heroku/dokku (im running RoR)

@bf4
Copy link

bf4 commented Sep 13, 2018

@wongy91

how do i verify if jemalloc is running on heroku/dokku (im running RoR)

That's really off topic. I'm pretty sure that the right way to get help for this buildpack, assuming you're using it, is to 1) look for an existing issue 2) create a new one showing what you've tried, what you see, and what you expected

@dbackeus
Copy link

Currently it's about a 50/50 split in popularity between this original repo and @gaffneyc's fork looking at heroku buildpacks list:
https://elements.heroku.com/search/buildpacks?q=jemalloc

Would be nice to reduce confusion and fragmentation of usage by deprecating one of the repos. Most straight-forward way would seem to be to get things merged into original repo rather than switching to the fork?

@gaffneyc
Copy link
Collaborator Author

tldr; Migrate to gaffneyc/heroku-buildpack-jemalloc if you want newer versions of jemalloc. Or stick with this buildpack if things are stable for you.

Since opening this PR almost 11 months ago I haven't had much time to revisit it. Even with #19 there is still work to be done to finish the migration and an inherent risk to anyone depending on this buildpack that a major change like this could break their applications on the next deploy.

I'm committed to maintaining my fork since we depend on it for Dead Man's Snitch and several client projects at Collective Idea. With limited time the idea of maintaining two branches of the same code isn't very enticing and it's shown over the last year as I've been more responsive on my fork to issues.

Given the risk to people using this buildpack and the reality of the time I have available, I'm closing this PR (sorry :/) and recommending that people migrate to my fork as they're able to. This means stability for anyone who's happy with the current buildpack but it also means the migration will be opt-in, giving teams the ability to choose when and how they handle it.

@gaffneyc gaffneyc closed this Oct 30, 2018
gaffneyc added a commit that referenced this pull request Aug 6, 2019
It has been a couple years since the last update and there have been a
couple jemalloc releases since then. I've been maintaining my fork for a
couple years now and just added 5.2.1.

This buildpack has had a great life (and should continue to work as is)
but I think it's worth it to steer folks toward a maintained fork. For
example, it was recommended for Ruby to skip the 4.x branch and to stick
to 3.6.0 (released 2015), but our team has had really good luck with
5.1.0 and 5.2.0.

There was a pull request to merge the fork into the buildpack but given
the risk it would expose anyone using the current buildpack to I don't
think it is a good path forward. For more details see this comment:
#18 (comment)
gaffneyc added a commit that referenced this pull request Aug 6, 2019
It has been a few years since the last update and jemalloc has had
several releases in that time. I've been maintaining my fork and
provided support on it for a couple years now and I think it's time to
deprecate this one.

This buildpack has had a great life (and should continue to work as is)
but I think it's worth it to steer folks toward a maintained fork. For
example, it was recommended for Ruby to skip the 4.x branch and to stick
to 3.6.0 (released in 2015), but our team has had really good luck with
5.1.0 and 5.2.0.

There was a [pull request](#18)
to merge the fork into the buildpack but given the risk it would expose
anyone using the current buildpack to I don't think it is a good path
forward. For more details see this comment:
#18 (comment)
@gaffneyc gaffneyc deleted the upgrade-jemalloc branch May 20, 2020 20:07
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.

10 participants