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

rustc: Allow changing the default allocator #27400

Merged
merged 1 commit into from Aug 15, 2015

Conversation

alexcrichton
Copy link
Member

This commit is an implementation of RFC 1183 which allows swapping out
the default allocator on nightly Rust. No new stable surface area should be
added as a part of this commit.

Two new attributes have been added to the compiler:

  • #![needs_allocator] - this is used by liballoc (and likely only liballoc) to
    indicate that it requires an allocator crate to be in scope.
  • #![allocator] - this is a indicator that the crate is an allocator which can
    satisfy the needs_allocator attribute above.

The ABI of the allocator crate is defined to be a set of symbols that implement
the standard Rust allocation/deallocation functions. The symbols are not
currently checked for exhaustiveness or typechecked. There are also a number of
restrictions on these crates:

  • An allocator crate cannot transitively depend on a crate that is flagged as
    needing an allocator (e.g. allocator crates can't depend on liballoc).
  • There can only be one explicitly linked allocator in a final image.
  • If no allocator is explicitly requested one will be injected on behalf of the
    compiler. Binaries and Rust dylibs will use jemalloc by default where
    available and staticlibs/other dylibs will use the system allocator by
    default.

Two allocators are provided by the distribution by default, alloc_system and
alloc_jemalloc which operate as advertised.

Closes #27389

@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @brson

cc @nnethercote

@rust-highfive rust-highfive assigned brson and unassigned pcwalton Jul 30, 2015
@alexcrichton alexcrichton force-pushed the less-jemalloc branch 2 times, most recently from 3e5fd87 to 8517834 Compare July 30, 2015 16:54
@@ -49,15 +49,16 @@ impl<'a, 'v> visit::Visitor<'v> for CrateReader<'a> {
}

fn dump_crates(cstore: &CStore) {
debug!("resolved crates:");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the switch from debug! to info! (and likewise from log::DEBUG to log::INFO) deliberate, or an accident?

I would have thought we could (and should) leave it as it was, under debug!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was currently intentional but I wouldn't mind changing it back. Whenever someone's having weird behavior with loading crates I typically ask them to set RUST_LOG=rustc::metadata::creader,rustc::metadata::loader to get info like this, but debug! means it's all compiled out by default.

Along those lines I'd somewhat prefer to have it be info! but I don't have much of a preference either way

@nnethercote
Copy link
Contributor

@alexcrichton: can I ask what the code would look like for a program (such as Servo) to implement allocation wrappers? I know we talked about this at Whistler but I don't remember the details and now that you have an implementation to reference it might be easier to describe. Thank you.

@alexcrichton
Copy link
Member Author

Certainly! You'd basically just create a copy of src/liballoc_jemalloc/lib.rs, filling in different implementations for the various allocation functions. You'd then just say extern crate foo to link in your allocator and you'd be good to go!

mod imp {
use core::mem::size_of;
use libc::{BOOL, DWORD, HANDLE, LPVOID, SIZE_T, INVALID_HANDLE_VALUE};
use libc::{WriteFile};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WriteFile and INVALID_HANDLE_VALUE aren't needed.


#[cfg(windows)]
mod imp {
use core::mem::size_of;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_of is also unused it seems.

@brson
Copy link
Contributor

brson commented Jul 31, 2015

This feels very complex, makes me want to remove jemalloc.

@retep998
Copy link
Member

Considering how jemalloc is responsible for #26647 and is also really brittle when used under conemu #14600 (comment).

You know, this reminds me of the time we had green threaded IO, then split it using virtual dispatch between a green and a native portion, then later dropped the green part entirely.

@steveklabnik
Copy link
Member

steveklabnik commented Jul 31, 2015 via email

@alexcrichton
Copy link
Member Author

@brson

This feels very complex, makes me want to remove jemalloc.

Could you elaborate a bit on this? I can certainly more aggressively document various aspects and I certainly wouldn't mind rewriting various code paths. I don't think that we want to just look at this and jettison jemalloc, though, as there are very real other use cases for swapping out for custom allocators (such as Servo's memory tracking, embedded systems with different allocators, etc).

If we followed reasoning like this I would also say that supporting both dylibs/rlibs at the same time is pretty complex, but I don't feel the need to remove that distinction. There's concrete use cases for both so we just need to support both in as robust a manner as possible.


@retep998

You know, this reminds me of the time we had green threaded IO, then split it using virtual dispatch between a green and a native portion, then later dropped the green part entirely.

I don't think that this is the same situation as here most notably because there's no virtual dispatch here. This is a static decision which is made that has no costs associated with it (unlike the virtual dispatch).

Like I said above, complexity is not a reason to jettison something, it's a reason to rethink why it's so complex and consider other options, but in this case I don't consider removing jemalloc an option.

If there are bugs on Windows caused by our usage of jemalloc (I was unaware of these), then we should track them down and fix them, but removing it from all platforms is a pretty heavy hammer.

@alexcrichton
Copy link
Member Author

I'm currently running a crater build for this PR

@alexcrichton
Copy link
Member Author

Crater reports one regression but I have confirmed locally that it was spurious.

@nnethercote
Copy link
Contributor

A few comments...

  • The ability to provide custom allocators is very important and useful.
  • In the context of Rust, I've heard it said many times that jemalloc is faster, but I haven't seen any measurements showing this. (Hmm, now that I think of it, somebody may have mentioned that using jemalloc sped up rustc significantly? Not sure.)
  • Servo uses a mixture of jemalloc (for Rust code) and the system heap (for C/C++ code, e.g. SpiderMonkey). Servo's memory reporting code thus has machinery for dealing with both: it reports the total size of both heaps, and it uses je_malloc_usable_size to measure jemalloc heap blocks and malloc_usable_size (on Linux) or malloc_size (on Mac) to measure system heap blocks.

@bors
Copy link
Contributor

bors commented Aug 3, 2015

☔ The latest upstream changes (presumably #27210) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Aug 5, 2015

☔ The latest upstream changes (presumably #27393) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton force-pushed the less-jemalloc branch 2 times, most recently from 9e1e50d to 49632c0 Compare August 5, 2015 05:11
@bors
Copy link
Contributor

bors commented Aug 5, 2015

☔ The latest upstream changes (presumably #27458) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member Author

@bors: r=brson abd839

@bors
Copy link
Contributor

bors commented Aug 13, 2015

🙀 abd839 is not a valid commit SHA. Please try again with adbd839.

@alexcrichton
Copy link
Member Author

@bors: r=brson adbd839

@bors
Copy link
Contributor

bors commented Aug 14, 2015

🔒 Merge conflict

@bors
Copy link
Contributor

bors commented Aug 14, 2015

☔ The latest upstream changes (presumably #27684) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member Author

@bors: r=brson 9d21851

@bors
Copy link
Contributor

bors commented Aug 14, 2015

⌛ Testing commit 9d21851 with merge 7959211...

@bors
Copy link
Contributor

bors commented Aug 14, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member Author

@bors: retry

On Fri, Aug 14, 2015 at 12:19 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-x-android-t
http://buildbot.rust-lang.org/builders/auto-linux-64-x-android-t/builds/6061


Reply to this email directly or view it on GitHub
#27400 (comment).

@bors
Copy link
Contributor

bors commented Aug 14, 2015

⌛ Testing commit 9d21851 with merge d5f5dca...

@bors
Copy link
Contributor

bors commented Aug 14, 2015

💔 Test failed - auto-linux-64-x-android-t

This commit is an implementation of [RFC 1183][rfc] which allows swapping out
the default allocator on nightly Rust. No new stable surface area should be
added as a part of this commit.

[rfc]: rust-lang/rfcs#1183

Two new attributes have been added to the compiler:

* `#![needs_allocator]` - this is used by liballoc (and likely only liballoc) to
  indicate that it requires an allocator crate to be in scope.
* `#![allocator]` - this is a indicator that the crate is an allocator which can
  satisfy the `needs_allocator` attribute above.

The ABI of the allocator crate is defined to be a set of symbols that implement
the standard Rust allocation/deallocation functions. The symbols are not
currently checked for exhaustiveness or typechecked. There are also a number of
restrictions on these crates:

* An allocator crate cannot transitively depend on a crate that is flagged as
  needing an allocator (e.g. allocator crates can't depend on liballoc).
* There can only be one explicitly linked allocator in a final image.
* If no allocator is explicitly requested one will be injected on behalf of the
  compiler. Binaries and Rust dylibs will use jemalloc by default where
  available and staticlibs/other dylibs will use the system allocator by
  default.

Two allocators are provided by the distribution by default, `alloc_system` and
`alloc_jemalloc` which operate as advertised.

Closes rust-lang#27389
@alexcrichton
Copy link
Member Author

@bors: r=brson 45bf1ed

@bors
Copy link
Contributor

bors commented Aug 14, 2015

⌛ Testing commit 45bf1ed with merge 290cf6b...

@alexcrichton
Copy link
Member Author

@bors: retry force

bors added a commit that referenced this pull request Aug 14, 2015
This commit is an implementation of [RFC 1183][rfc] which allows swapping out
the default allocator on nightly Rust. No new stable surface area should be
added as a part of this commit.

[rfc]: rust-lang/rfcs#1183

Two new attributes have been added to the compiler:

* `#![needs_allocator]` - this is used by liballoc (and likely only liballoc) to
  indicate that it requires an allocator crate to be in scope.
* `#![allocator]` - this is a indicator that the crate is an allocator which can
  satisfy the `needs_allocator` attribute above.

The ABI of the allocator crate is defined to be a set of symbols that implement
the standard Rust allocation/deallocation functions. The symbols are not
currently checked for exhaustiveness or typechecked. There are also a number of
restrictions on these crates:

* An allocator crate cannot transitively depend on a crate that is flagged as
  needing an allocator (e.g. allocator crates can't depend on liballoc).
* There can only be one explicitly linked allocator in a final image.
* If no allocator is explicitly requested one will be injected on behalf of the
  compiler. Binaries and Rust dylibs will use jemalloc by default where
  available and staticlibs/other dylibs will use the system allocator by
  default.

Two allocators are provided by the distribution by default, `alloc_system` and
`alloc_jemalloc` which operate as advertised.

Closes #27389
@bors
Copy link
Contributor

bors commented Aug 14, 2015

⌛ Testing commit 45bf1ed with merge ab450ef...

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.

None yet