Skip to content

Rewrite build system for new versions and Heroku stacks#14

Closed
gaffneyc wants to merge 1 commit into
sethfitz:jemalloc-5.0.0from
gaffneyc:jemalloc-5.0.0
Closed

Rewrite build system for new versions and Heroku stacks#14
gaffneyc wants to merge 1 commit into
sethfitz:jemalloc-5.0.0from
gaffneyc:jemalloc-5.0.0

Conversation

@gaffneyc

@gaffneyc gaffneyc commented Dec 5, 2017

Copy link
Copy Markdown
Collaborator

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

@gaffneyc

gaffneyc commented Dec 5, 2017

Copy link
Copy Markdown
Collaborator Author

This is the first part of some work I've been doing today. Started as a review of what was here and... kind of spiraled. I'm also looking at making changes to bin/compile but wanted to discuss them before getting too deep into it.

The goal here was to make it easy to create builds for new versions of jemalloc. The next step that I'm working on is being able to use those builds and easily switch between them. From a high level this codebase is two things: heroku buildpack for downloading compiled binaries and tooling to build those binaries.

It should be possible to quickly and easily make binaries available based on version without needing to update the buildpack. I'm thinking we can use a set of releases (one per stack) that hosts the binaries for each version of jemalloc (I have a proof of concept on my fork).

For switching between versions, I've been able to have a JEMALLOC_VERSION env variable that will compile different versions when a new slug is compiled. I am also considering a JEMALLOC_ENABLED flag that will automate the process of setting LD_PRELOAD.

@gaffneyc

gaffneyc commented Dec 5, 2017

Copy link
Copy Markdown
Collaborator Author

In bin/compile is it necessary to set LD_LIBRARY_PATH, LIBRARY_PATH, PKG_CONFIG, CPPPATH, and CPATH? I believe those are only necessary if someone plans to link against jemalloc but only LD_PRELOAD is required for using the library. Removing them from bin/compile would reduce the file quite a bit.

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
@csuhta

csuhta commented Dec 5, 2017

Copy link
Copy Markdown

👍 on the environment variables to control the build and if jemalloc should be used. That makes testing very easy too.

Using jemalloc.sh is fairly awkward right now.

@gaffneyc

gaffneyc commented Dec 5, 2017

Copy link
Copy Markdown
Collaborator Author

I've fleshed out what I was thinking and it's sitting on master of my fork. We're slowly testing it on a couple apps and already have it running on Dead Man's Snitch.

Going to clean up some of the fork specific changes and get another PR up for consideration.

@csuhta

csuhta commented Dec 8, 2017

Copy link
Copy Markdown

@gaffneyc I was trying out your folk, and I noticed the builds for jemalloc 3.6.0 do not include a jemalloc-config script in the archive

@gaffneyc

gaffneyc commented Dec 8, 2017

Copy link
Copy Markdown
Collaborator Author

@csuhta Interesting, I will take a look. I'm not modifying the sources at all so I'm guessing it's not part of the 3.x line. I'll see about getting a work around in place.

Going to take some time today to upstream my changes as well.

@csuhta

csuhta commented Dec 8, 2017

Copy link
Copy Markdown

@gaffneyc

gaffneyc commented Dec 8, 2017

Copy link
Copy Markdown
Collaborator Author

Was there a config change you needed to make for it to be generated or did you add the file directly?

@csuhta

csuhta commented Dec 8, 2017

Copy link
Copy Markdown

I manually created them

@gaffneyc

gaffneyc commented Dec 8, 2017

Copy link
Copy Markdown
Collaborator Author

Gotcha. It looks like jemalloc-config was added in 4.0.0. Is it worth support any versions older than 4.0.0 other than 3.6.0?

@gaffneyc

gaffneyc commented Dec 8, 2017

Copy link
Copy Markdown
Collaborator Author

It may be possible to hardcode it as /app/vendor/jemalloc/lib/libjemalloc.so as it's a symlink to the specific revision. Since slugs are compiled independently on each code change, there should only ever be one version installed in /app/vendor/jemalloc

@gaffneyc

gaffneyc commented Dec 8, 2017

Copy link
Copy Markdown
Collaborator Author

@csuhta I've pushed gaffneyc@6564f30 which should fix the issue with 3.6.0. I was able to remove the need for jemalloc-config entirely.

Comment thread Makefile

.PHONY: cedar-14 heroku-16

# Build for cedar stack (deprecated)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cedar-14 isn't deprecated, can just remove this comment

@nateberkopec

Copy link
Copy Markdown
Collaborator

Thank you so much for this.

I am also considering a JEMALLOC_ENABLED flag that will automate the process of setting LD_PRELOAD.

I like this, but can leave for another day/PR.

@nateberkopec

Copy link
Copy Markdown
Collaborator

Maybe also for a different PR, but to use any of this properly, we'll have to change the way this line works in bin/compile, correct?

@gaffneyc

gaffneyc commented Dec 11, 2017

Copy link
Copy Markdown
Collaborator Author

I've been planning to create a PR for what's on the master branch of my fork but have been pressed for time this last week. Thinking that PR would supersede this one and probably #11.

@gaffneyc

Copy link
Copy Markdown
Collaborator Author

@gaffneyc

Copy link
Copy Markdown
Collaborator Author

Took a couple minutes and submitted #18 which supersedes this one.

@gaffneyc gaffneyc closed this Dec 11, 2017
@gaffneyc gaffneyc deleted the jemalloc-5.0.0 branch December 11, 2017 21:52
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