correctly drop Array::IntoIter

[Radvendii]

Since #1683 has been open a while, and I'm not sure when I'm going to have the time to work out what's going on with Interner, I wanted to separate out the changes to Array::IntoIter.

If you called Array::into_iter(), you could leak elements in two circumstances:

1. If you did not iterate through all elements before dropping, since they had been converted to ManuallyDrop
2. If the Array had multiple references when the iterator was created, but the other references were dropped before the iterator was dropped, then the existence of the iterator would prevent those Rcs from dropping their contents, but then because the iterator is of type ManuallyDrop, it wouldn't drop those elements either

[yannham]

Missing safety comment (but beside other remarks/questions, LGTM)

[yannham]

Revisiting this, I think like we're going through a lot of trouble for no obvious gain. IIUC, what this iterator does is to drop stuff live as it is iterating if we're the only reference to the backing array. I guess the original motivation was that you could be the only remaining live reference, but have a very small view into the array (say, the [10,15] interval in a 10k elements array).

But, I wonder if we should really care at this point about such optimizations (we can wait for the iterator be consumed or dropped). Besides, I'm more concerned with running time than with memory consumption for now. And it's not obvious (or even probably false in a lot of common cases) than dropping things in many steps is faster than dropping everything at once.

This is why I'm tempted to get rid of this complexity altogether and just a normal iterator. What do you think @Radvendii ?

[Radvendii]

IIUC, what this iterator does is to drop stuff live as it is iterating if we're the only reference to the backing array.

I thought the point was more that if you're, say, mapping over an Array and you're the only reference, you don't have to clone all the elements, you can modify-in-place. But I'm not super confident in that understanding.

This is why I'm tempted to get rid of this complexity altogether and just a normal iterator.

Oh, hmm... I'm not opposed. What's the status of testing the real-world speed? Can we run that medium-sized code base someone provided and see if simplifying this to a normal iterator slows it down?

[yannham]

I thought the point was more that if you're, say, mapping over an Array and you're the only reference, you don't have to clone all the elements, you can modify-in-place. But I'm not super confident in that understanding.

Like this, from the top of my head, I would say that the fact that we can consume the backing memory of the array and reuse the allocation doesn't depend on this code, as this code seems to be concerned with dropping the individual elements or not, not the enclosing array. Given that we have a hand-written iterator, I wonder if there's any chance that array.into_iter().map(f).collect() can reuse the inner array - I can't see how the compiler could do so. But I must admit I don't have a good idea of how it works for Rust stdlib structures like Vec, so let me investigate a bit. That might actually be a better performance goal (than cloning or not the individual elements).

As for individual elements, it's true that the current code avoid cloning - but honestly, cloning terms is cheap. Unless it prevents some reuse-in-place later, maybe, but at that point the iterator should have been consumed and thus the Rc count be decremented. Though, about the initial motivation, maybe you're right: ManuallyDrop might have been used just for ManuallyDrop::take in next(), more than for early dropping of terms.

Oh, hmm... I'm not opposed. What's the status of testing the real-world speed? Can we run that medium-sized code base someone provided and see if simplifying this to a normal iterator slows it down?

Yes, I have a couple sizeable real world codebases, although they have been given to me as private, I can try with and without and see if there's any noticeable difference. With the proviso that variance is always huge - I've always suspected the hashmap randomization and the fact that we stress hashmaps a lot, but have never come to actually test this hypothesis.

Thanks for your input. I think the situation isn't all black-and-white, so while waiting for further investigations on whether the current implementation is worth it, or if we could do better with respect to mutating-in-place, I don't see a good reason to block this PR which strictly improves the current situation.