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

Provide feature to use nightly allocator API #6

Open
NOBLES5E opened this issue Nov 28, 2020 · 14 comments
Open

Provide feature to use nightly allocator API #6

NOBLES5E opened this issue Nov 28, 2020 · 14 comments

Comments

@NOBLES5E
Copy link

In latest Rust nightly we can allocate with custom allocator (for example https://doc.rust-lang.org/nightly/alloc/vec/struct.Vec.html#method.new_in).

It will be great for bumpalo-herd to provide the ability to use this. To achieve this, we need to implement std::alloc::AllocRef.

See also fitzgen/bumpalo#87.

@vorner
Copy link
Owner

vorner commented Nov 28, 2020

I'm not opposed to having this, under a feature (maybe allocator-api-unstable or something?). But I don't know when exactly I would get to it (not a feature I'd use myself right now), so would you want to submit a PR?

@NOBLES5E
Copy link
Author

I gave it a try today, and it seems like we need change on bumpalo first, and there is already a PR fitzgen/bumpalo#68

@NOBLES5E
Copy link
Author

NOBLES5E commented Feb 9, 2021

@vorner bumpalo has merged allocator API: fitzgen/bumpalo#92

@vorner
Copy link
Owner

vorner commented Feb 10, 2021

Nice :-). Do you want to give the PR a try?

@wackbyte
Copy link
Contributor

wackbyte commented Sep 1, 2023

I think I'll try to add this :)

@wackbyte
Copy link
Contributor

wackbyte commented Sep 1, 2023

Hmm, I don't think it would be possible to implement this in a way to have the allocator shared between threads since the collection must keep a reference to the allocator (Member is not Sync; Herd is, but it cannot provide the exact Bump the collection was allocated with). However, it would still be nice to have a feature flag that forwards to bumpalo's own allocator_api feature flag.

@vorner
Copy link
Owner

vorner commented Sep 4, 2023

I wonder… the fact we don't remember the exact Bump, is it a problem? I mean, I'd suspect the deallocate on the Bump arena is empty, because Bump can't deallocate, it can be only completely cleaned. So, can we just forward all allocation requests to arbitrary subarena?

@wackbyte
Copy link
Contributor

wackbyte commented Sep 4, 2023

I checked, and Bump's implementation of deallocate checks if the pointer was the last allocation in the arena and reuses the memory if it was.

@vorner
Copy link
Owner

vorner commented Sep 8, 2023

I see. But it „leaks“ in all the other cases, so we could just leak every time.

Honestly, just forwarding the feature is kind of… not really much useful. You can always just list both bumpalo and bumpalo-herd in the Cargo.toml, the former with the feature.

simnalamburt added a commit to contentstech-com/bumpalo-herd that referenced this issue Oct 28, 2024
@simnalamburt
Copy link
Contributor

@vorner @wackbyte I’ve created a proof of concept (PoC) for implementing the Allocator API. I would appreciate it if you could review it and share your thoughts.

References

@ejmount
Copy link

ejmount commented Nov 27, 2024

I am also interested in being able to use a Member as an allocator for other collections, and #15 looked exactly what would be needed to be able to do that. Would someone be able to explain why that was closed?

@simnalamburt
Copy link
Contributor

#15 (review) This was the reason.

If you need Member as an allocator, you can just use Member::as_bump()

@ejmount
Copy link

ejmount commented Dec 1, 2024

Apologies for the previous comment being unclear, I didn't have a lot of time to spend on it.

First of all, while I saw that comment previously, I'd missed the distinction between the Allocator being implemented for Herd vs Member.

I saw as_bump, but the specific use I was aiming for was being able to say Vec<_, &Member>. This turned out not to solve my problem even after I borrowed the PR to build against, but my thought was that Vec<_, &Bump> by way of as_bump would have different semantics because the allocations would be tied to the &Bump, which will have a shorter lifetime. I've since realized that while that is technically true, I can't think of a situation where a Vec<_, &Member> is more flexible than Vec<_, &Bump> because the former will still be constrained to the scope of, e.g. a single thread that called get(), so that thread can then just also call as_bump().

Is there any reason brought up that Herd cannot directly implement Allocator? I read the previous comments as suggesting that the behaviour might be suboptimal but otherwise work correctly. If not, I'll have a look at raising a PR to do so.

@simnalamburt
Copy link
Contributor

This is how I perceive this issue:

  • For Vec<T> where T: !Send, the implementation is relatively straightforward, but it’s almost identical to Vec<_, &Bump> with Member::as_bump().
  • For Vec<T> where T: Send, I’m uncertain whether it’s feasible to implement it properly. Since multiple containers allocated within a single arena should be able to be sent to multiple different destinations, there’s a concern about how it will be dropped.

But still, if you have an idea of how to approach this, I'll gladly review it! Please enlighten us :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants