Optimize empty case in Vec::retain

[jcsp]

While profiling some code that happens to call Vec::retain() in a tight loop, I noticed more runtime than expected in retain, even in a bench case where the vector was always empty. When I wrapped my call to retain in if !myvec.is_empty() I saw faster execution compared with doing retain on an empty vector.

On closer inspection, Vec::retain is doing set_len(0) on itself even when the vector is empty, and then resetting the length again in BackshiftOnDrop::drop.

Unscientific screengrab of a flamegraph illustrating how we end up spending time in set_len and drop:

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nilstrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[conradludgate]

Slightly more scientific research shows that llvm can produce less instructions with this is_empty check included.

https://godbolt.org/z/jz9Eoqfbc

From looking at it, it is already compiling to a:

        mov     r14, qword ptr [rdi + 16]
        test    r14, r14
        je      .LBB1_1

which tests if the vec len is 0. Except this LBB1_1 label does not immediately return and does some other routine instead before finally returning. This label is not reached from any other path in the assembly which looks like a missed optimisation.

[tgross35]

Good find! Slightly better view of the asm changes https://godbolt.org/z/r3YvP4s5e, seems to shave about 15% of the assembly on x86, 6% on aarch64.

One request above then LGTM. (please squash back into one commit after you add a comment)

[tgross35]

@rustbot author

[jcsp]

@rustbot review

[tgross35]

Thanks!

r? @tgross35
@bors r+

[bors]

📌 Commit 6a6824a has been approved by tgross35

It is now in the queue for this repository.

[klensy]

rollup=never ?

[matthiaskrgr]

@bors rollup=never perf

[bors]

⌛ Testing commit 6a6824a with merge dba8e2d...

[bors]

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing dba8e2d to master...

[rust-timer]

Finished benchmarking commit (dba8e2d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -3.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.094s -> 770.597s (0.07%)
Artifact size: 331.91 MiB -> 331.91 MiB (0.00%)