jmcomets / tcmalloc-rs

A drop-in GlobalAlloc implementation using tcmalloc from gperftools.
https://crates.io/crates/tcmalloc
Apache License 2.0
16 stars 7 forks source link

Build and link to tcmalloc statically #3

Closed brson closed 5 years ago

brson commented 5 years ago

This makes gperftools a submodule, adds a tcmalloc-sys crate, and builds and links to tcmalloc.

If somebody really wants to use a local tcmalloc they can enable the no-build feature.

Note that I've only tested on x86_64-linux-unknown-gnu, and so I whitelisted that target in the build script — for any other target the script will fail.

This also bumps the version number. If you accept this PR I hope you will publish a new revision.

jmcomets commented 5 years ago

Thanks for the PR! I do have a few minor review notes on the build.rs script.

Aside from the code itself, I'm wondering if a submodule is the right way to go here. For example, tensorflow-sys clones the repository and checks out the specific tag before building from there. If you have any extra info on that, I'd be interested, but I'll accept it anyways.

brson commented 5 years ago

@jmcomets Re submodule vs cloning during build, I am mostly copying from jemallocator here. The main reason to use a submodule and not do a clone that I can think of is that git clone only works for branches and tags, not arbitrary commits. But gperftools is well tagged so that doesn't really apply. I will change it to do a clone.

brson commented 5 years ago

Thanks for the review.

I pushed a rebase that addresses all comments.

I deleted info! in favor of println!. "bundled" is now not the default, and the tcmalloc-sys crate isn't used when not "bundled".

I also removed the version bump. Since this is adding an optional feature I don't think a major bump is necessary. You can handle the versions.

jmcomets commented 5 years ago

This looks great and works out of the box, well done. :+1:

jmcomets commented 5 years ago

FYI there may be some notification spam here while I get this published to crates.io, don't mind it too much