Remove special-case handling of vec.split_off(0)

[Zalathar]

#76682 added special handling to Vec::split_off for the case where at == 0. Instead of copying the vector's contents into a freshly-allocated vector and returning it, the special-case code steals the old vector's allocation, and replaces it with a new (empty) buffer with the same capacity.

That eliminates the need to copy the existing elements, but comes at a surprising cost, as seen in #119913. The returned vector's capacity is no longer determined by the size of its contents (as would be expected for a freshly-allocated vector), and instead uses the full capacity of the old vector.

In cases where the capacity is large but the size is small, that results in a much larger capacity than would be expected from reading the documentation of split_off. This is especially bad when split_off is called in a loop (to recycle a buffer), and the returned vectors have a wide variety of lengths.

I believe it's better to remove the special-case code, and treat at == 0 just like any other value:

• The current documentation states that split_off returns a “newly allocated vector”, which is not actually true in the current implementation when at == 0.
• If the value of at could be non-zero at runtime, then the caller has already agreed to the cost of a full memcpy of the taken elements in the general case. Avoiding that copy would be nice if it were close to free, but the different handling of capacity means that it is not.
• If the caller specifically wants to avoid copying in the case where at == 0, they can easily implement that behaviour themselves using mem::replace.

Fixes #119913.

[rustbot]

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

[the8472]

That eliminates the need to copy the existing elements, but comes at a surprising cost

That isn't really a cost since it's the same old allocation. I'd say the extra cost is the with_capacity call in for the now-empty self.

The current documentation states that split_off returns a “newly allocated vector”

😮‍💨 I wish people stopped documenting implementation details that aren't necessary to the function of a method.
But yes, given that existing documentation it makes sense.

I think we should ping @richkadel though, perhaps he can give arguments in favor of the existing code.

Also note that we now have mem::take() which is a simpler alternative to mem::replace().

[Zalathar]

That isn't really a cost since it's the same old allocation. I'd say the extra cost is the with_capacity call in for the now-empty self.

The “cost” that I have in mind is not just the larger allocation for self, but also the fact that the returned vector might have far more capacity than is necessary to hold its contents.

In many cases, that cost won't matter. The returned vector might happen to be more than half-full, or it might be very short-lived, or it might be only a negligible fraction of the overall program's memory usage. But if split_off is called in a loop, and the size of the vector is often much less than its capacity, then that bloat can start to add up.

(If the user knows this quirk of the implementation, and wants to avoid it, they may end up having to copy the returned vector into a new vector with a more appropriate capacity. Doing so completely undermines the benefit of trying to avoid the copy in the first place.)

If the standard library decides to incur that extra cost on the user's behalf in the at == 0 case, we need to be confident that the benefit (avoiding memcpy) is worth it. I don't think we can justify that confidence.

(I think it's also significant that this choice of implementation only comes up for the at == 0 case. For any other value, the only reasonable implementation is to allocate a sensibly-sized buffer and memcpy the taken elements. That in turn makes it surprising that the at == 0 case might do something dramatically different.)

Also note that we now have mem::take() which is a simpler alternative to mem::replace().

Curiously, mem::take was stabilized in late 2019, but this special case was added in late 2020.

Calling split_off(0) is logically equivalent to calling mem::take, except for how it handles allocation/capacity. So this seems like another reason to avoid handling capacity in a non-obvious way.

[richkadel]

Thank you for identifying this issue and raising some great points!

I really appreciate that @Zalathar took the time to read through my original defense in #76682, so I don't need to reiterate everything.

I do want to raise awareness (for anyone that didn't read through that PR) that there were rudimentary tests done to show the performance benefit of the change, for the use case I was trying to solve. So to revert it now will negatively impact existing code that currently depends on the behavior from 2021 until now.

In the PR, we did discuss the fact that the rustdoc for split_off() is underspecified, which I believe is still a problem.

I did mention mem::replace() in the PR, and I made the (at the time, unchallenged) argument that mem:: functions were not commonly used except in more advanced code. Neither that, nor mem::take(), sprung to mind, to me, at that time, regardless of when it was stablized. It just wasn't a common pattern that I saw being used for cases like these, in most of the compiler and operating system code I was working on at the time. The point being, someone can look at split_off() and its current rustdoc, and assume it would be just as efficient as mem::take(), and they'd have been very wrong. It just wasn't intuitive.

So to simply revert it puts us back in what I think is still a less than ideal place.

But I 100% agree that the capacity issues of both the initial value and the returned value are problematic.

My opinion is just mine, so I hope you get input from a few others, including @Mark-Simulacrum who approved the original change, but I suggest:

• Either, don't revert the behavior (so existing code performance is not degraded), BUT update the rustdoc to explain its expected/permitted/intended(?) behavior(s) in the case of split_off(0) so a reader at least has a better chance of understanding the trade-offs, and the opportunity to decide if this is the right function for them;
• OR, revert it (accept a modified version of this PR) but include changes to the rustdoc that warns that the implementation of split_off(0) copies the entire buffer into a new Vec, and recommends considering mem::take() or mem::replace() (replace would allow controlling the initial capacity).

If you change it, I think it's especially important to update the rustdoc this time around. Recommending the mem:: functions will really benefit developers considering these use cases.

[the8472]

There are other possible approaches such as only moving the allocation when it's not significantly oversized.

[Zalathar]

If you change it, I think it's especially important to update the rustdoc this time around. Recommending the mem:: functions will really benefit developers considering these use cases.

Yeah, regardless of whether this PR is accepted or not, it would be good to add suggestions for other similar options, especially in the 0 case:

• mem::take will steal the entire contents+buffer, and avoids allocating a replacement capacity
• mem::replace will steal the entire contents+buffer, and gives explicit control over what is left behind in its place
• Vec::drain can remove an arbitrary slice, not just a suffix

Also as mentioned above, the current implementation is inconsistent with the “newly allocated vector” wording, so that tension ought to be resolved in some way.

[Zalathar]

I have also found that https://doc.rust-lang.org/std/collections/index.html#sequences lists the performance of split_off(i) as O(n-i), which is inconsistent with the current implementation, as the capacity can be arbitrarily larger than n.

[the8472]

As long as there is no call to realloc happening capacity shouldn't factor into performance since requesting uninitialized memory from the OS doesn't depend on the size of the allocation.

[cuviper]

I think this makes sense, especially with doc additions to discuss the various capacity options. But it's enough of a change to warrant talking as a team.

@rustbot +I-libs-nominated

[Zalathar]

This was raised in https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202024-01-17/near/416062684, but that particular discussion didn't reach a specific conclusion/consensus before moving on to other nominated issues.

[cuviper]

After a followup discussion, and with the docs already added in #120180, I think we're good!

@bors r+

[bors]

📌 Commit a655558 has been approved by cuviper

It is now in the queue for this repository.