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

'unsafe impl<A> Allocator for &A where A: Allocator' is problematic #98232

Open
RalfJung opened this issue Jun 18, 2022 · 3 comments
Open

'unsafe impl<A> Allocator for &A where A: Allocator' is problematic #98232

RalfJung opened this issue Jun 18, 2022 · 3 comments
Labels
A-allocators Area: Custom and system allocators I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

We have a general Allocator instance for &A where A: Allocator. I think that is a problem. In #98178 I fixed a problem where BTreeMap<T, Global> would actually use &Global as allocator, which leads to performance regression. Or at least, I thought I had fixed that problem -- I just realized that there are some places where it still adds an indirection and therefore adds an unnecessary reference that will be copied around:

self.range.deallocating_end(&self.alloc);

I found this by removing the unsafe impl<A> Allocator for &A where A: Allocator + ?Sized instance. I don't know of another way to avoid accidentally introducing such performance regressions.

So maybe that instance should be removed?
Cc @rust-lang/wg-allocators

@RalfJung RalfJung changed the title 'unsafe impl<A> Allocator for &A where A: Allocator + ?Sized' is problematic 'unsafe impl<A> Allocator for &A where A: Allocator' is problematic Jun 18, 2022
@CAD97
Copy link
Contributor

CAD97 commented Jun 18, 2022

See also #94069, #94114 (comment); there're other reasons that this blanket implementation is problematic.

I'd personally be fine removing the impl for the short-to-medium term at least. Long-term/stable needs a bit more consideration at least, though.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 19, 2022
Remove accidental uses of `&A: Allocator`

Cc rust-lang#98232

Fixes rust-lang#98176 (for real this time)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 19, 2022
Remove accidental uses of `&A: Allocator`

Cc rust-lang#98232

Fixes rust-lang#98176 (for real this time)
@Amanieu
Copy link
Member

Amanieu commented Jun 24, 2022

@CAD97 I don't think it's problematic if we fix the wording on the Allocator trait as I suggested in #94114 (comment).

@CAD97
Copy link
Contributor

CAD97 commented Jun 24, 2022

The impl can be made sound via doc clarifications (which should certainly happen even without the impl provided), yes; I was just noting the issue as a related incidental complexity of the impl.

workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Remove accidental uses of `&A: Allocator`

Cc rust-lang/rust#98232

Fixes rust-lang/rust#98176 (for real this time)
@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. A-allocators Area: Custom and system allocators T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants