btree: avoid forcing the allocator to be a reference

[RalfJung]

The previous code forces the actual allocator used to be some &A. This generalizes the code to allow any A: Copy. If people truly want to use a reference, they can use &A themselves.

Fixes #98176

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[RalfJung]

Ah there are still some & somewhere that create a non-ZST allocator. Not sure what the best way is to find them... but I think we should get rid of them all, since we want this to be truly zero-cost.

[Amanieu]

Could this be changed to require Clone instead of Copy? This is in line with what HashMap currently requires.

[RalfJung]

I figured Copy would be simpler and good enough since every allocator can be made Copy by using &A. But yeah it can probably be Clone. Not sure when I will get around to do that.

[thomcc]

I agree Clone sounds better, at least if it's what HashMap does.

@rustbot label +S-waiting-on-author -S-waiting-on-review

[RalfJung]

Okay I made it Clone. This is super annoying as many things need to be cloned manually, and I don't quite see how an allocator would be Clone but not Copy... I hope I did not introduce too many unnecessary clones, as those are hard to find. It is also very hard to be sure that there is not accidentally a &A somewhere. Having impl<A: Allocator> Allocator for &A is suboptimal here.

[thomcc]

I don't quite see how an allocator would be Clone but not Copy

It holding an Arc internally is a common example.

[TennyZhuang]

I figured Copy would be simpler and good enough since every allocator can be made Copy by using &A. But yeah it can probably be Clone. Not sure when I will get around to do that.

In fact, it's always hard to use &A, the main question is who owned the allocator, which has to introduce a self-referential struct. For convenience, we have to use an Arc<A>.

[RalfJung]

This is blocking getting Miri to work again (#98107), so would be good to get it landed soon. :)

[thomcc]

Thanks, this looks fine to me.

@bors r+

[bors]

📌 Commit 3a1e114 has been approved by thomcc

[RalfJung]

@bors p=1
unblocks Miri

[bors]

⌛ Testing commit 3a1e114 with merge ff86b27...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing ff86b27 to master...

[rust-timer]

Finished benchmarking commit (ff86b27): comparison url.

Instruction count

• Primary benchmarks: mixed results
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions 😿 (primary)	0.4%	0.4%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.0%	-2.0%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-0.8%	-2.0%	2

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	2.3%	3.0%	3
Regressions 😿 (secondary)	3.1%	3.1%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.3%	3.0%	3

Cycles

Results

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.6%	2.6%	1
Improvements 🎉 (primary)	-5.3%	-7.9%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-5.3%	-7.9%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[joboet]

The Clone bound concerns me, as it is called often, so it will impact performance for not-so-cheap clones like Arc. Also, it makes it impossible to store allocator data inline, which might be nice for things like slab-allocators in kernels. Couldn't this use a wrapper struct like this:

struct AllocRef<'a, A> {
    allocator: [&'a A; (size_of::<A>() > 0) as usize],
}

impl<'a, A: Allocator> Deref for AllocRef<'a, A> {
    type Target = A;

    fn deref(&self) -> &A {
        // This should be fine?
        self.allocator.get(0).unwrap_or_else(|| unsafe { NonNull::dangling().as_ref() })
    }
}

impl<'a, A> Copy for AllocRef<'a, A> {}

impl<'a, A> Clone for AllocRef<'a, A> {
    ...
}

which preserves the size of ZSTs but doesn't unnecessarily require Clone.

[RalfJung]

This sounds like something a user of BTreeMap / HashMap could also do?

[joboet]

This sounds like something a user of BTreeMap / HashMap could also do?

No, because that would require the allocator to be a reference. The problem is that some allocators cannot be cloned, but the BTreeMap should own the allocator. For example, if I used a bump allocator to speed up allocations, I cannot return the map.

If something like AllocRef were used internally to refer to the allocator, only one instance of the allocator would need to be stored.

[thomcc]

@joboet That's very clever. Are you interested in submitting a PR for it?

[RalfJung]

No, because that would require the allocator to be a reference. The problem is that some allocators cannot be cloned, but the BTreeMap should own the allocator. For example, if I used a bump allocator to speed up allocations, I cannot return the map.

Oh I see, so you want BTreeMap to own the allocator but the Boxes it creates to just borrow it? I thought the idea was the A would just be propagated everywhere, but sure -- as long as ZST allocators lead only to ZST allocators, I think this should all be fine.

[ssomers]

Allowing BTreeMap to own any kind of allocator is one thing, but when the BTreeMap temporarily passes its allocator to internal helper functions, does it have to be cloned? Isn't that what call-by-reference is meant to do?

[RalfJung]

If that internal helper function creates a Box<T, A>, you want to maintain the zero-sizedness of A.

IOW, for the default allocator Global, clone is actually cheaper than pass-by-reference: Global has size 0, so it will be compiled away entirely; &Global is 8 bytes large and those will be passed around through the stack of helper functions.

[ssomers]

I understand. But then I don't understand the use of Allocator + Clone everywhere and not Allocator + Copy (obviously, it would be better without Clone but I sure don't know how). An allocator is used for efficiency, as far as I know. A allocator used in a BTreeMap (and I assume in hashbrown too) can only be used efficiently if it is cheap to clone, because cloning happens all over the place. If people are prepared to implement Clone but not to commit to Copy, why lure them into using it in a map?

[RalfJung]

Yeah, well, I was in favor of Allocator + Copy but I was asked to use Clone instead so I did. This is consistent with what HashMap does. I don't know if HashMap also needs to do as much cloning as BTreeMap.

Anyway discussions on closed PRs are not usually a good idea, so it'd probably be better if you opened a new issue for this.

[TennyZhuang]

I found it’s really hard to implement an allocator with reference but without Arc. The struct who owned the map will also owns the allocator itself, which means that it’s unavoidable to introduce Pin, pin_project, and many other complicated and even unsafe codes to implement the self-referential data structure. Using Arc here at least can make it simple and clear.