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

Separate dealloc from AllocRef into DeallocRef #69065

Closed
wants to merge 1 commit into from

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Feb 11, 2020

This creates a new trait DeallocRef, makes it a supertrait of AllocRef and moves dealloc into the new trait.

With this way, it is possible to have allocators, which cannot allocate new memory, but free old one. This comes in handy when dealing with FFI interfaces, which returns a pointer and a free function.

For a full discussion on this topic please see rust-lang/wg-allocators#9

r? @Amanieu

closes rust-lang/wg-allocators#9

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2020
@Amanieu
Copy link
Member

Amanieu commented Feb 12, 2020

I am still somewhat hesitant to merge this since I feel that the use cases are limited (basically just Box) and not worth the added complexity for implementing and using an allocator (2 traits instead of 1).

cc @rust-lang/libs

@bjorn3
Copy link
Member

bjorn3 commented Feb 12, 2020

The current name sounds like it only supports deallocation, not allocation.

@SimonSapin
Copy link
Contributor

SimonSapin commented Feb 12, 2020

I have two objections to this PR, and feel it should not land as-is or at all:

The only reason to have separate traits is to have separate types that implement them: A: AllocRef and D: DeallocRef. The idea is that if you give up on Clone, after you’ve created it a Box<T, D> only ever needs to deallocate. For some very niche allocators, D can be made zero-size even if A cannot, allowing Box<T, D> to be no larger than *mut T.

(If A can be made zero-size too such as in a global allocator, there is no need for separate types or traits.)

But how do you create that Box<T, D> in the first place? It needs to be allocated at some point. Box::new_in needs an A: AllocRef to allocate, and returns something that contains A. So some other API on Box (and any other container where one wants to take advantage of this trait separation) is missing.

Just passing two parameters alloc: A, dealloc: D is risky: we need to make sure they actually reference the same allocator "instance". Should that Box constructor be an unsafe fn?

Or maybe there could be some kind of of "downgrade" operation to convert a value of A to one of D. This might require an associated type somewhere.

Either way, PR is useless without some other APIs that don’t even have a sketch of a design proposal.


This makes significant API complexity: two traits, an associated type to convert between them, corresponding constructors on containers. All only relevant if:

  • You have an allocator where A: AllocRef is not zero-size. Meaning it is not global, but supports multiple "instances" within one process, so you need some kind of way to disambiguate which instance to allocate in.
  • Somehow, deallocation does not need to be told which allocator instance to deallocate from. Maybe dealloc is a no-op, or it can figure it out based on the address of the data pointer.
  • You are willing to give up on Clone, or somehow never need to (re)allocate a container after its initial creation.
  • You have many such collections, enough that reducing Box<T: Sized, D: DeallocRef> from 16 bytes to 8 bytes is worth the trouble of using dedicated APIs. (Remember that Vec<T, Global> is 24 bytes, and the vast majority of its users don’t feel the need to reach out to https://crates.io/crates/thin-vec)

Everyone would else still need to deal with the complexity of separate traits when reading https://doc.rust-lang.org/std/alloc/

By the way, is anyone involved here actually using an allocator like this?


How did we get here? A separate Dealloc has been discussed for years but there never was a complete proposal of how it would be used. I think that when designing an API we sometimes enjoy thinking about how to make it maximally general in order to support every conceivable use case, while being maximally performant. But we should remember to balance that with other aspects that make an API good: limited complexity budget, learnability, ease of use, …

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2020
@bors
Copy link
Contributor

bors commented Mar 3, 2020

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

@TimDiekmann
Copy link
Member Author

As there are too many unresolved question as pointed out by @SimonSapin I'll close this. For further discussion on this topic, I guess the WG repository is better suited.

@TimDiekmann TimDiekmann closed this Mar 4, 2020
@TimDiekmann TimDiekmann deleted the deallocref branch August 4, 2020 17:27
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 25, 2023
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.

Separate dealloc from Alloc into other trait
8 participants