search

fitzgen / bumpalo

A fast bump allocation arena for Rust
https://docs.rs/bumpalo
Apache License 2.0
1.42k stars 112 forks source link

Update `Alloc` trait to nightly, add nightly feature #68

Closed hansihe closed 3 years ago

hansihe commented 4 years ago
fitzgen commented 4 years ago

It looks like AllocRef isn't found in nightly CI: https://dev.azure.com/fitzgen/bumpalo/_build/results?buildId=161&view=logs&j=4348da94-6ff4-557c-1304-492c812b9157&t=db45a008-f7f6-5c94-4ab6-81ac5572c26f&l=224

Looks like we're getting rustc 1.41 from rustup: https://dev.azure.com/fitzgen/bumpalo/_build/results?buildId=161&view=logs&j=4348da94-6ff4-557c-1304-492c812b9157&t=a4ec2f30-9a1e-5354-6437-3d76911eae85&l=12

Is AllocRef in 1.41?

hansihe commented 4 years ago

I meant to get back to this earlier when I saw that the build failed.

It looks like --default-toolchain $RUSTUP_TOOLCHAIN might not be respected in https://github.com/fitzgen/bumpalo/blob/master/ci/install-rust.yml#L4? Even though nightly is passed to rust_version, stable is installed.

I'll poke around a bit

hansihe commented 4 years ago

Okey, so it seems like the alloc trait is under major flux right this moment.

I guess it would make sense to wait until things have settled a bit to update and merge?

I presume what you want to do is to track the (feature flagged) alloc trait present in the stable releases?

hansihe commented 4 years ago

Yeah, let's hold off on this until most changes to the AllocRef (formerly alloc) trait are out of the way.

https://github.com/rust-lang/hashbrown/pull/133#issuecomment-595413077

fitzgen commented 4 years ago

Sounds like a plan!

NOBLES5E commented 3 years ago

Sounds like a plan!

AllocRef is on nightly now. Let's start working on merging this? :blush:

fitzgen commented 3 years ago

AllocRef is on nightly now. Let's start working on merging this? blush

Sounds good. Unclear to me whether this PR should be revived or whether starting from scratch would be easier. I don't have time to drive it forward, but I can review the eventual implementation.

NOBLES5E commented 3 years ago

AllocRef is on nightly now. Let's start working on merging this? blush

Sounds good. Unclear to me whether this PR should be revived or whether starting from scratch would be easier. I don't have time to drive it forward, but I can review the eventual implementation.

It seems that this PR only misses the latest two commits (which only contains some modification in the readme file).

@hansihe What do you think? Let's revive this PR?

hansihe commented 3 years ago

Both Box and Vec in nightly have an allocator parameter now. hashbrown also has an allocator parameter on both HashMap and HashSet, and they should land on their std counterparts soon too.

For me at least, things in the Rust ecosystem have progressed to the point where I would be fine with bumpalo just providing the Bump arena, along with an AllocRef implementation.

My fork, located here, updates the traits to latest nightly, but also removes all collections from the codebase.

It's understandable if @fitzgen would prefer to keep the collections in the crate for now. If that's the case, I will be maintaining my fork as the effort is pretty minimal. If someone else would like to contribute an updated collections implementation on top of my fork in order to get things merged back here, I would be really happy with that too.

fitzgen commented 3 years ago

Yes, I'd like to keep the collections module until the custom allocators ship on stable.

In the meantime, I'd still like to have an AllocRef/Allocator/whatever-it-ends-up-being-called implementation behind an unstable cargo flag. If we can lift that from your fork, that would be great.

I would be fine with bumpalo just providing the Bump arena, along with an AllocRef implementation.

Yep, this is the eventual goal but we need custom allocators on stable before we remove the collections module.

(I don't mind you maintaining a fork, or course, but out of curiosity: what would your fork have over the main crate other than removing the collections module? Since the collections module is off by default and requires a cargo flag to enable, it doeTsn't seem like forking is really getting you that much, unless there are additional changes.)

hansihe commented 3 years ago

I just don't have the time right now to update the different collections to the newest version of the trait, so I won't be able to make a PR that includes that. If this repo lands an update to the trait, then that's brilliant and I'll of course switch to using that directly instead.

fitzgen commented 3 years ago

Fixed in #92.