Skip to content

Don’t assume MIN_ALIGN for small sizes #25

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

Merged
merged 1 commit into from
Nov 24, 2017

Conversation

SimonSapin
Copy link
Contributor

Same as rust-lang/rust#46117

See also discussion of jemalloc’s behavior at jemalloc/jemalloc#1072

Same as rust-lang/rust#46117

See also discussion of jemalloc’s behavior at
jemalloc/jemalloc#1072
@gnzlbg
Copy link
Owner

gnzlbg commented Nov 23, 2017

@SimonSapin could you check if this use case is covered in #26 ?

If so, it might be good to run the benches on this PR and see if there is a difference, and if so, how big.

But AFAIK this is required for correctness, is that right?

@SimonSapin
Copy link
Contributor Author

Right, this is correctness fix. The aspect that’s performance-dependent is whether we should go further and remove MIN_ALIGN entirely.

@gnzlbg
Copy link
Owner

gnzlbg commented Nov 23, 2017

Makes sense.


FWIW MIN_ALIGN is also used here to dispatch to calloc instead of mallocx(size, flags | MALLOCX_ZEROED), so it might have performance implications beyond the cost of align_to_flags.

@SimonSapin
Copy link
Contributor Author

Right, calling calloc is probably similar to the flags == 0 fast path in other jemalloc functions discussed in rust-lang/rust#46117 (comment).

@gnzlbg
Copy link
Owner

gnzlbg commented Nov 23, 2017

@SimonSapin yes I've added a benches/calloc.rs that benchmarks calloc vs mallocx( , flags | MALLOCX_ZEROED), the differences in most cases is "small", in some cases mallocx is faster and in others calloc is faster, but you will be able to try for yourself once that is merged.

Please feel free to review the benchmarks and give me input. Its a lot of boilerplate that I don't seem to be able to avoid because I cannot concatenate identifiers to form new identifiers in macros.

@alexcrichton alexcrichton merged commit 6d557d3 into gnzlbg:master Nov 24, 2017
@alexcrichton
Copy link
Collaborator

Thanks!

@SimonSapin SimonSapin deleted the min-align branch November 24, 2017 17:18
BusyJay added a commit to BusyJay/jemallocator that referenced this pull request Apr 29, 2022
I will also publish this crate in the name of jemallocator. Because tikv-jemallocator is already used by several projects, to make less disturbing to dependents, two crates will be published at the same time with only name differences.

The tikv-xxx versions are maintained in the branch tikv-main.

Signed-off-by: Jay Lee <[email protected]>
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