Specialize arena alloc for cloned slices

[bugadani]

Copy contents of a slice.iter().cloned() directly into the arena. Also replace implementation of write_from_iter to something that can maybe be optimized better: https://rust.godbolt.org/z/qox784

Let me know if I did something entirely terribly incorrect with the custom Clonable specialization marker, I'm not sure if it's valid to add stuff like that.

[rust-highfive]

r? @varkor

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

[jyn514]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit 9ab93f2 with merge 691e8249eca167a156669b07665b8be442f00e7c...

[the8472]

My somewhat limited understanding of the min_specialization rules is that you're here promising that any specialization relying on Clonable will only be applied to types where T: Clone is not dependent lifetimes (directly or indirectly)
E.g. that there's no impl Clone for &'static Foo involved. That can only be guaranteed if you could also manually enumerate all T, i.e. they're not types supplied by external crates.

It would also mean that it's unsafe to use TypedArena for such types and users of it would have to uphold that in the future.

[bugadani]

I'm not quite sure if this is the case, but I don't know the feature well enough. I was going by the example of FusedIterator and TrustedLen, both of which are implemented for e.g. std::slice::Iterator<'a, T: 'a> and I believe both are specialization marker traits.

[the8472]

The thing is that FusedIterator or TrustedLen are not implemented for arbitrary T. They're only implemented for structs controlled by std and those impl are only conditional on additional bounds that themselves are other specialization traits.

What's different here is that you're essentially declaring that any and all Clone types can act as a specialization marker for specializations relying on Clonable. This would only be valid when you have some way of reasoning about the set of T. And since T is what goes into TypedArena it means you've ensured that all uses of TypedArena are compatible with the min specialization rules. This kind of far-reaching obligation is why there's the word unsafe in that annotation.

But again, my understanding of min_specialization is quite limited, so I may be wrong here.

[bugadani]

All right, makes sense. I wish I had another way to specialize for Cloned<slice::Iter> :( Whatever else I tried, I got a conflicting implementation error....

But even then, perf may get back completely negative or neutral as well.

[bors]

☀️ Try build successful - checks-actions
Build commit: 691e8249eca167a156669b07665b8be442f00e7c (691e8249eca167a156669b07665b8be442f00e7c)

[rust-timer]

Queued 691e8249eca167a156669b07665b8be442f00e7c with parent bc39d4d, future comparison URL.

@rustbot label: +S-waiting-on-perf

[the8472]

This doesn't seem to rely on anything specific to Cloned. It just relies on the length. Wouldn't specializing on TrustedLen be sufficient here, considering that's already a specialization trait?

[bugadani]

No, it wouldn't compile unfortunately, or at least I couldn't get it to. Cloned needs Clone on T.

[bugadani]

Also, just specializing on anything TrustedLen is unsound, see #77945 for that. In short, Map is TrustedLen and can allocate on the same arena, which makes handling a panic safely an impossible nightmare of dropping uninitialized memory.

I went through the uses of alloc_from_iter and I found that most of those are Map and FlatMap, but some places allocate from Clonedslice::Iter so hence the PR.

[the8472]

I meant impl<I: TrustedLen> IterExt for I. That'll give you the length.

[the8472]

Well it's closed already. But for the record, you might have been able to use use impl<I: TrustedLen + IsClonedSliceIterMarker> for I instead where the latter bound is a safe specialization marker.

[bugadani]

Indeed, this seems to compile:

#[rustc_specialization_trait]
trait IsClonedSliceIterMarker {}
impl<T> IsClonedSliceIterMarker for Cloned<Iter<'_, T>> {}

impl<T, I> IterExt<T> for I
where
    I: IntoIterator<Item = T> + IsClonedSliceIterMarker + TrustedLen,
{
    #[inline]
    fn alloc_from_iter(self, arena: &TypedArena<T>) -> &mut [T] {
        let iter = self.into_iter();
        let len = iter.size_hint().0;
        if len == 0 {
            return &mut [];
        }
        // Move the content to the arena by copying and then forgetting it
        unsafe {
            let start_ptr = arena.alloc_raw_slice(len);
            write_from_iter(iter, len, start_ptr)
        }
    }
}

[rust-timer]

Finished benchmarking try commit (691e8249eca167a156669b07665b8be442f00e7c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[jyn514]

Looks like noise. Given the specialization is in question, I think it makes sense to close this.