move mallocx_align to jemalloc-sys and expose it via the ffi module

[gnzlbg]

No description provided.

[alexcrichton]

How come? Ideally this crate is pretty conservative in its API, so why would we expose this?

[gnzlbg]

This crates exposes the ffi module, and the functions in utils are necessary to use the ffi module properly:

• mallocx_align is a re-implementation of a C macro that the C interface exposes
• align_to_flags is what everybody using the C interface needs

@SimonSapin suggested removing MIN_ALIGN completely, so maybe we shouldn't expose those, but at least align_to_flags is a must, otherwise everybody needs to reimplement it, and that would be out-of-sync with @SimonSapin changes already... it is better to provide it here.

[SimonSapin]

MIN_ALIGN and align_to_flags implement a specific, opinionated optimization strategy. The ffi-level interface can definitely be used without them.

mallocx_align could maybe be moved to the jemalloc-sys crate, since it is equivalent to something in the C API.

[SimonSapin]

If the MIN_ALIGN strategy is dropped, calls to align_to_flags would be replaced with calls to mallocx_align.

[gnzlbg]

@SimonSapin doesn't it still make sense to expose a fn size_align_to_flags(size, align) -> c_int API that does the same that the library internally does? (Be it use MIN_ALIGN or not?)

One might want to compute the "alignment" like jemalloctor does internally, but bitwise-or it with other jemalloc flags. That is actually the main reason I see for using the C-API directly instead of just the Allocator API.

---

I'll move mallocx_align to the jemalloc-sys crate, and drop the rest here, since that seems to be a non-controversial change. But I think that a size/align to jemalloc flags utility function makes sense not only internally but also in the public API.

[SimonSapin]

One might want to compute the "alignment" like jemalloctor does internally, but bitwise-or it with other jemalloc flags.

That is what mallocx_align does.

[gnzlbg]

Not really, at least not with the current state of affairs where we use the MIN_ALIGN and the size, mallocx_align uses either of those :/ I am not saying we must expose this api “as is” but maybe we should improve the API and expose that instead. All allocation methods use align to flags right now.

…

On Fri 24. Nov 2017 at 18:03, Simon Sapin ***@***.***> wrote: One might want to compute the "alignment" like jemalloctor does internally, but bitwise-or it with other jemalloc flags. That is what mallocx_align does. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/alexcrichton/jemallocator/pull/27#issuecomment-346869685>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NpnE-WNoa3I6g6E7--6HbiXe7NI2Pks5s5vacgaJpZM4Qor4r> .

[SimonSapin]

Replicating the exact flag-setting strategy of jemallocator while not using jemallocator is a choice you can make, it is not a "must". And I’m not convinced it’s a good idea: it was buggy (rust-lang/rust#45955) at least since https://github.com/alexcrichton/jemallocator/pull/25, and even now its validity is shaky.

[gnzlbg]

I agree that it is not an easy choice but iff we could at least offer a safe choice that would already be worth it. I mean that particular issue has shown that making a safe choice for Rust programs at all isn’t easy :/ expecting others to come with other safe choices that perform good is unrealistic, because it is very tricky to do so.

…

On Fri 24. Nov 2017 at 20:34, Simon Sapin ***@***.***> wrote: Replicating the exact flag-setting strategy of jemallocator while not using jemallocator is a choice you can make, it is not a "must". And I’m not convinced it’s a good idea: it was buggy (rust-lang/rust#45955 <rust-lang/rust#45955>) at least since #25 <https://github.com/alexcrichton/jemallocator/pull/25>, and even now its validity is shaky. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/alexcrichton/jemallocator/pull/27#issuecomment-346887349>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NptEj9e23-nNi7OJLtaKWndy-fX6Hks5s5xpjgaJpZM4Qor4r> .

[gnzlbg]

So i've just moved mallocx_align to the jemalloc-sys crate, which makes jemallocator re-export it via the FFI module and removed all other changes from this PR.

#35 renames align_to_flags to layout_to_flags, it might make sense or not to expose an API for getting sensible jemalloc flags from a layout, but I'll open an issue for that.

[alexcrichton]

If this is a macro can it have the same name as C?

[gnzlbg]

Alex in C it has the same name but all in caps, I am so sorry about the amount at pull requests. I did not realize it was thanks giving over there and I am fully booked till tomorrow so I have only very squeezed in amounts of time in between to rebase them on master. Please hold on on everything that looks like an optimization till the benchmarks are merged, and also even though some PRs are green they might break each other once they get merged so it might be better to just retry each PR on Travis after each merge. I tried to set up appveyor this morning but I couldn’t finish that before I left :/ I’ll try to do that tomorrow.

…

On Sat 25. Nov 2017 at 16:13, Alex Crichton ***@***.***> wrote: If this is a macro can it have the same name as C? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/alexcrichton/jemallocator/pull/27#issuecomment-346946366>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NpswJ2qoBKwYVvRSvqgZyk30fiF5gks5s6C6UgaJpZM4Qor4r> .

[alexcrichton]

Ah yeah that's just what I mean about changing the name to MALLOCX_ALIGN to align with the macro in C (so it "looks like" the C interface)

Also no worries on all the PRs! It is a little overwhelming but just takes time to get through.

[gnzlbg]

@alexcrichton that's a good idea, I've named it all in caps like the C macro.

[alexcrichton]

Looks great!