heroku-buildpack-jemalloc icon indicating copy to clipboard operation
heroku-buildpack-jemalloc copied to clipboard

Upgrade to jemalloc 5.0.0

Open mojodna opened this issue 7 years ago • 9 comments

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 avatar Jun 15 '17 21:06 mojodna

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.)

mojodna avatar Jun 15 '17 21:06 mojodna

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 avatar Sep 16 '17 05:09 brian-kephart

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 avatar Sep 17 '17 04:09 brian-kephart

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.

brian-kephart avatar Sep 20 '17 19:09 brian-kephart

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

nateberkopec avatar Oct 03 '17 13:10 nateberkopec

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

nateberkopec avatar Oct 03 '17 13:10 nateberkopec

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

nateberkopec avatar Oct 03 '17 13:10 nateberkopec

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.

nateberkopec avatar Oct 03 '17 13:10 nateberkopec

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 #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.

brian-kephart avatar Oct 03 '17 17:10 brian-kephart