-
Notifications
You must be signed in to change notification settings - Fork 63
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
Document JEMALLOC_OVERRIDE #107
Conversation
Welcome @nmattia! It looks like this is your first PR to tikv/jemallocator 🎉 |
I'm unfortunately getting a 404 here: https://github.com/tikv/jemallocator/blob/master/CONTRIBUTING.md |
jemalloc-sys/README.md
Outdated
provide your own `jemalloc` version, set `JEMALLOC_OVERRIDE` to point to a built | ||
version of `jemalloc`. This can point to either a shared or static library. For | ||
instance: `JEMALLOC_OVERRIDE=/path/to/libjemalloc.a`. The library should be built | ||
with `jemalloc`'s `--with-jemalloc-prefix=_rjem_`. For static libraries it is also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not accurate. Whether prefix is required depends on whether unprefixed_malloc_on_supported_platforms
is enabled and whether the target is compatible with overwriting malloc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not accurate.
Right, though the default seems to be to use the prefixed version so I thought this was simpler. Should I mention the prefix logic here? I'd need to dig into it a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing it like following?
JEMALLOC_OVERRIDE
is for advanced usage only. Providing your own library means most features of jemalloc-sys will be ignored in build script, and you are responsible to compile the library to match what jemalloc-sys expects. Especially, handle API prefix as whatever feature unprefixed_malloc_on_supported_platforms
says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. PTAL @BusyJay !
Please fix DCO as https://github.com/tikv/jemallocator/pull/107/checks?check_run_id=32653878537 says. |
@BusyJay unfortunately I’m getting a 404. How do I sign it? |
The link I provided is different from the bot one. |
This adds a README paragraph explaining how to provider a prebuilt version of malloc through `JEMALLOC_OVERRIDE`, as well as build flags expected by `tikv-jemalloc-sys`. Signed-off-by: Nicolas Mattia <nicolas@nmattia.com>
44be6f1
to
9722530
Compare
@BusyJay got it:) thanks! |
Thank you! |
This adds a README paragraph explaining how to provider a prebuilt version of malloc through
JEMALLOC_OVERRIDE
, as well as build flags expected bytikv-jemalloc-sys
.