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

Upgrade to jemalloc 5.0.0 #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Upgrade to jemalloc 5.0.0 #11

wants to merge 6 commits into from

Conversation

mojodna
Copy link
Owner

@mojodna mojodna commented Jun 15, 2017

Originally from @brian-kephart

@nateberkopec: I'll leave this for you to merge and make live (but I'll create the release w/ a new binary).

@mojodna
Copy link
Owner Author

mojodna commented Jun 15, 2017

Ok, pushing the binary created a v5.0.0 tag from current master (not including this), so you'll probably want to clean things up when merging.

(Sorry if this hurt more than helped.)

@brian-kephart
Copy link

I think there's an issue with the 5.0.0 binary. I tried to use the buildpack from the jemalloc-5.0.0 branch and got this message when deploying:

remote: -----> jemalloc app detected
remote: -----> Vendoring binaries
remote:        Fetching https://github.com/mojodna/heroku-buildpack-jemalloc/releases/download/5.0.0/jemalloc-5.0.0-1.tar.gz
remote:
remote: gzip: stdin: not in gzip format
remote: tar: Child returned status 1
remote: tar: Error is not recoverable: exiting now
remote:  !     Push rejected, failed to compile jemalloc app.

@brian-kephart
Copy link

Filename was incorrect, missing a 'v'. The binary file is https://github.com/mojodna/heroku-buildpack-jemalloc/releases/download/v5.0.0/jemalloc-5.0.0-1.tar.gz

Correcting the filename removed the build error. However, the new binary does not have the file /app/vendor/jemalloc/lib/libjemalloc.so.1. So, still no good.

I tried setting the LD_PRELOAD value as recommended in the jemalloc documentation:
LD_PRELOAD=`jemalloc-config --libdir`/libjemalloc.so.`jemalloc-config --revision` app
Success! (the above path resolves to /app/vendor/jemalloc/lib/libjemalloc.so.2 in version 5)

...

Only, the above solution doesn't work with the earlier version of jemalloc. As far as I can tell, we have a breaking change. What works for using jemalloc 5 doesn't work for < 5 and vice versa.

If this PR is going to be merged into master, the documentation needs to be updated. 5.x requires /app/vendor/jemalloc/lib/libjemalloc.so.2. If that approach is taken, #10 should not be merged as it won't work with v5.

@brian-kephart
Copy link

Correction to the above: The file change in 5.0.0 doesn't appear to affect anyone who was using jemalloc via the Procfile script, since that refers to jemalloc.sh rather than /app/vendor/jemalloc/lib/libjemalloc.so.2. I successfully pushed an app to Heroku with jemalloc in the Procfile and all ran as expected.

It looks like v5 won't break anyone's Procfile after all. Sorry to cause confusion.

I can't verify the effect on those using LD_PRELOAD in heroku config, since that configuration never worked for me anyway.

@nateberkopec
Copy link
Collaborator

Closing in favor of @brian-kephart's other PR.

@nateberkopec
Copy link
Collaborator

Whoops, didn't see that his PR was on top of this one 😆

@nateberkopec nateberkopec reopened this Oct 3, 2017
@nateberkopec
Copy link
Collaborator

OK, I think this is good now. @brian-kephart can you give this branch a shot too?

@nateberkopec
Copy link
Collaborator

Once this gets merged I can figure out the build environment stuff again, hopefully we can push releases with the latest minor of 3.x -> 4.x -> 5.x.

The previous version added leading and trailing colons to these values if a variable had no assigned value before the buildpack script ran.
@brian-kephart
Copy link

I deployed using this branch and it works. Some notes:

  1. Setting LD_PRELOAD using heroku config works now. It never worked for me before.
  2. Typing heroku config:set LD_PRELOAD=`jemalloc-config --libdir`/libjemalloc.so.`jemalloc-config --revision` into the terminal resolves the path to /app/vendor/jemalloc/lib/libjemalloc.so.2 before storing it. Since the bash commands in that line may have different results for different versions of jemalloc, users may have to reconfigure heroku config when new versions are released.
  3. It also occurs to me that since the path is resolved in the terminal before storing in heroku config, this command depends on the version of jemalloc installed on your local machine. That might break things. Can anyone verify?
  4. I changed LD_PRELOAD to `jemalloc-config --libdir`/libjemalloc.so.`jemalloc-config --revision` in the Heroku dashboard. This caused the app errors seen in LD_PRELOAD Causes Error on Deployment #6.
  5. If this gets merged into master, the config line in the documentation doesn't work below version 5, and this should be mentioned in the documentation for those using previous versions.

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.

3 participants