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

feat: use same allocator for all #1946

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

yangby-cryptape
Copy link
Collaborator

Notice

This PR can be reviewed but it requires test before merge.

Issue

Currently, ckb has two allocators:

  • When we compile the latest version CKB on Linux, the code written by Rust will use Jemalloc.

ckb/Cargo.toml

Lines 72 to 73 in dad394e

[target.'cfg(all(not(target_env = "msvc"), not(target_os="macos")))'.dependencies]
jemallocator = { version = "0.3.0" }

ckb/src/main.rs

Lines 4 to 6 in dad394e

#[cfg(all(not(target_env = "msvc"), not(target_os = "macos")))]
#[global_allocator]
static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;

  • But the code written by C will use system allocator, since the jemalloc will compiled with --with-jemalloc-prefix=_rjem_,

    • unprefixed_malloc_on_supported_platforms: when disabled, configure
      jemalloc with --with-jemalloc-prefix=_rjem_. Enabling this causes symbols
      like malloc to be emitted without a prefix, overriding the ones defined by
      libc. This usually causes C and C++ code linked in the same program to use
      jemalloc as well. On some platforms prefixes are always used because
      unprefixing is known to cause segfaults due to allocator mismatches.

Solution

We can enable feature unprefixed_malloc_on_supported_platforms to let symbols like malloc to be emitted without a prefix, overriding the ones defined by libc.

@yangby-cryptape yangby-cryptape requested review from a team and xxuejie February 19, 2020 09:40
@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Feb 24, 2020
CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Feb 25, 2020
@yangby-cryptape yangby-cryptape added s:bench-needed Status: Need to run benchmark s:hold Status: Put this issue on hold. labels Feb 25, 2020
@yangby-cryptape
Copy link
Collaborator Author

Hold for tests or benchmark.

@doitian doitian added s:hold Status: Put this issue on hold. and removed s:hold Status: Put this issue on hold. labels Mar 2, 2020
@doitian doitian moved this from ✅ Reviewer approved to ✋ Hold in CKB - Pull Requests Mar 3, 2020
@zhangsoledad
Copy link
Member

@zhangsoledad zhangsoledad removed s:bench-needed Status: Need to run benchmark s:hold Status: Put this issue on hold. labels Mar 11, 2020
@zhangsoledad zhangsoledad moved this from ✋ Hold to ✅ Reviewer approved in CKB - Pull Requests Mar 11, 2020
@zhangsoledad
Copy link
Member

bors r+

@nervos-bot nervos-bot bot added the s:ready-to-merge Status: Waiting to be merged. label Mar 11, 2020
@bors
Copy link
Contributor

bors bot commented Mar 11, 2020

Build succeeded

@bors bors bot merged commit 27c36a5 into nervosnetwork:develop Mar 11, 2020
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Mar 11, 2020
@yangby-cryptape yangby-cryptape deleted the pr/unify-allocator branch April 1, 2020 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:ready-to-merge Status: Waiting to be merged.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants