Use try_fold in Vec::extend_desugared#156440
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Tested locally: |
|
Vec code is very hot and highly responsive to perturbations, let's see what perf says @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use `try_fold` in `Vec::extend_desugared`
This comment has been minimized.
This comment has been minimized.
|
Error occured while categorizing benchmark run:
|
|
@rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
@rust-timer build dc931d4 |
|
Failed to enqueue some commit SHAs. Maybe they were already benchmarked? |
|
Scratch that. Let's start over. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use `try_fold` in `Vec::extend_desugared`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8902743): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.8%, secondary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 16.7%, secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 1.2%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 507.838s -> 524.419s (3.27%) |
| self.set_len(len + 1); | ||
| let (lower, _) = iterator.size_hint(); | ||
| self.reserve(lower); | ||
| while let Err(element) = iterator.try_fold((), |(), element| { |
There was a problem hiding this comment.
Vec::push_within_capacity does the same and avoids some unsafe.
|
You can see in the diff of the codegen profiles (direct link) that something about this likely causes CGUs to shuffle such that the incr-println scenario now invalidates a bunch more LLVM CGUs, resulting in a ton more work. I haven't yet had a chance to look at the implementation delta, but generally speaking I'm not particularly worried about the perf result there -- it's a fairly random effect most likely. |
| let (lower, _) = iterator.size_hint(); | ||
| self.reserve(lower); | ||
| while let Err(element) = iterator.try_fold((), |(), element| { | ||
| if self.len < self.capacity() { |
There was a problem hiding this comment.
Can we check the codegen for this? I'd expect it to help to make len and (probably) capacity local variables; I'd guess as-is LLVM can't tell the iterator and writing into the Vec won't mutate either.
This is probably a bit incompatible with the function suggestion above without a custom struct + fnmut impl (probably not worth it).
| let (lower, _) = iterator.size_hint(); | ||
| self.reserve(lower); | ||
| while let Err(element) = iterator.try_fold((), |(), element| { | ||
| if self.len < self.capacity() { |
There was a problem hiding this comment.
Is there a reason we're changing the condition from equality to < here? It seems likely that equality gives more room for optimization.
| self.set_len(len + 1); | ||
| let (lower, _) = iterator.size_hint(); | ||
| self.reserve(lower); | ||
| while let Err(element) = iterator.try_fold((), |(), element| { |
There was a problem hiding this comment.
I think we could have an inner function for this closure so that it is not instantiated for every passed iterator I, only every element type T. That should also help with the perf.rlo result I think?
The function may still want to be #[inline] (we should probably try both).
|
Reminder, once the PR becomes ready for a review, use |
View all comments
Try to revive #68046.
Resolves #63340.
Vec::extend_desugaredis the fallback path forcollect&extendfrom non-TrustedLeniterators. It previously usedwhile let Some(element) = iterator.next(), which makes adapters likeChainto execute extra code on every call tonext().This PR switches to
try_fold, which lets adapters use their specialized fold implementations, avoiding overhead.