Skip to content
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

Add bindings to all jemalloc public functions #21

Merged
merged 3 commits into from
Oct 30, 2017

Conversation

SimonSapin
Copy link
Contributor

That is, those documented in http://jemalloc.net/jemalloc.3.html

For example malloc and free can be useful when implementing callbacks for a C library that doesn’t provide a size when deallocating: https://www.freetype.org/freetype2/docs/reference/ft2-system_interface.html

The documented order is (number of objects, size of each object),
though in practice they end up being multiplied together
and so are somewhat interchangeable.
bors-servo referenced this pull request in servo/servo Oct 30, 2017
FreeType: don’t use usable_size() as deallocation size

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes #19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21
@alexcrichton
Copy link
Collaborator

Thanks! The signature of sallocx may be off though?

@SimonSapin
Copy link
Contributor Author

It’s quite possible since I translated from C declarations on the man page by hand, but this one seems to match?

    #[link_name = "_rjem_sallocx"]
    pub fn sallocx(ptr: *mut c_void, flags: c_int) -> size_t;

http://jemalloc.net/jemalloc.3.html

size_t sallocx( void *ptr,
                int flags);

@alexcrichton
Copy link
Collaborator

No idea myself, I'm just the messenger saying that CI is red

@SimonSapin
Copy link
Contributor Author

This a warning-treated-as-error in C code that this PR didn’t modify. Maybe Travis updated their compiler version? Does this error happen on master if you hit the retry button?

@alexcrichton
Copy link
Collaborator

Mind looking into this? This is a test verifying the C API is correct. You added bindings for sallocx in this PR, and the C compiler is saying it's not correct.

@SimonSapin
Copy link
Contributor Author

Oh sorry, I thought this C code was part of jemalloc, I didn’t realize it was generated from Rust declarations. Looking.

That is, those documented in http://jemalloc.net/jemalloc.3.html

For example `malloc` and `free` can be useful when implementing
callbacks for a C library that doesn’t provide a size when deallocating:
https://www.freetype.org/freetype2/docs/reference/ft2-system_interface.html
@SimonSapin
Copy link
Contributor Author

Alright, pushed with a fix. The problem was that man page says void *ptr (which translates to *mut c_void) but the code says const void *ptr (which translates to *const c_void).

bors-servo referenced this pull request in servo/servo Oct 30, 2017
FreeType: don’t use usable_size() as deallocation size

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes #19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19061)
<!-- Reviewable:end -->
@alexcrichton
Copy link
Collaborator

I'm a tad bit wary about jemalloc::ffi, but perhaps instead of pub extern crate it could look like:

pub mod ffi {
    pub use jemalloc_sys::*;
}

?

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Oct 30, 2017

IIRC pub extern crate has been doing the same thing as that mod + pub use pattern since rust-lang/rust#31362.

@alexcrichton
Copy link
Collaborator

Ok... Can the version bumps be left out? I'm sort of wary with a release-per-PR, so maybe something like [patch] can be used to test these out and then they'll get published later?

@SimonSapin
Copy link
Contributor Author

I’ve already made a Servo try build using these changes through [patch]. Everything is green other than a tidy failure in my Servo PR. In general we avoid landing usage of [patch] or [replace] in Servo master: this is effectively forking a library from upstream which is more of a last-resort option.

I can remove the version increment if you prefer. I’ve included it to signal that I’d appreciate a crates.io version including these changes to be published at your convenience. This doesn’t necessarily mean a separate version for every single PR, batching them is totally fine. A new version would enable this Servo PR to land as-is (with [patch] removed), and hopefully fix a segfault on Android. If you say that publishing needs to wait on something that’s likely to take time, I can look for alternatives.

@alexcrichton
Copy link
Collaborator

Nah it's fine to have, I just wanted to make sure that it was tested before release. So many PRs on other projects tend to have 3 successive releases in a row as they were never tested...

In any case, thanks again!

@alexcrichton alexcrichton merged commit db270c4 into gnzlbg:master Oct 30, 2017
@SimonSapin SimonSapin deleted the bind-all-the-functions branch October 30, 2017 17:19
bors-servo referenced this pull request in servo/servo Oct 30, 2017
FreeType: don’t use usable_size() as deallocation size

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes #19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19061)
<!-- Reviewable:end -->
moz-v2v-gh referenced this pull request in mozilla/gecko-dev Oct 31, 2017
…on size (from servo:ft-alloc); r=mbrubeck

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes servo/servo#19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

Source-Repo: https://github.com/servo/servo
Source-Revision: f18099118a5be17b5b1d6fdcc3352a98a1499e6a

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 8087ee658a0ca822c6cdc85c00cfc8984b1fa668
xeonchen referenced this pull request in xeonchen/gecko-cinnabar Oct 31, 2017
…on size (from servo:ft-alloc); r=mbrubeck

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes servo/servo#19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

Source-Repo: https://github.com/servo/servo
Source-Revision: f18099118a5be17b5b1d6fdcc3352a98a1499e6a
gecko-dev-updater referenced this pull request in marco-c/gecko-dev-wordified-and-comments-removed Oct 2, 2019
…on size (from servo:ft-alloc); r=mbrubeck

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes servo/servo#19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

Source-Repo: https://github.com/servo/servo
Source-Revision: f18099118a5be17b5b1d6fdcc3352a98a1499e6a

UltraBlame original commit: 7f574eb1f99cc0561b58c045a8cba0552e7ee930
gecko-dev-updater referenced this pull request in marco-c/gecko-dev-comments-removed Oct 2, 2019
…on size (from servo:ft-alloc); r=mbrubeck

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes servo/servo#19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

Source-Repo: https://github.com/servo/servo
Source-Revision: f18099118a5be17b5b1d6fdcc3352a98a1499e6a

UltraBlame original commit: 7f574eb1f99cc0561b58c045a8cba0552e7ee930
gecko-dev-updater referenced this pull request in marco-c/gecko-dev-wordified Oct 2, 2019
…on size (from servo:ft-alloc); r=mbrubeck

Instead use C-level malloc()/free() so that the size doesn’t need to be known during deallocation, since FreeType doesn’t provide it.

Hopefully fixes servo/servo#19058

Depends on https://github.com/alexcrichton/jemallocator/pull/21

Source-Repo: https://github.com/servo/servo
Source-Revision: f18099118a5be17b5b1d6fdcc3352a98a1499e6a

UltraBlame original commit: 7f574eb1f99cc0561b58c045a8cba0552e7ee930
BusyJay added a commit to BusyJay/jemallocator that referenced this pull request Apr 13, 2022
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
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.

2 participants