Use try_fold in Vec::extend_desugared

[el-ev]

View all comments

Try to revive #68046.
Resolves #63340.

Vec::extend_desugared is the fallback path for collect&extend from non-TrustedLen iterators. It previously used while let Some(element) = iterator.next(), which makes adapters like Chain to execute extra code on every call to next().

This PR switches to try_fold, which lets adapters use their specialized fold implementations, avoiding overhead.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 7 candidates
• Random selection from Mark-Simulacrum, nia-e

[el-ev]

Tested locally:

bench_extend_chain_filter                                                                                                                                                    
                            main (ns)   HEAD (ns)   change                                                                                                                     
    0000_0010                  189.88      158.91     -16%                                                                                                                     
    0000_0100                  724.40      339.66     -53%                                                                                                                     
    0000_1000                 5106.20     1248.02     -76%                                                                                                                     
    0010_0010                  186.01      128.01     -31%                                                                                                                     
    0100_0100                  602.00      205.02     -66%                                                                                                                     
    1000_1000                 5251.13     1167.29     -78%
                                                                                                                                                                               
bench_collect_chain_filter                                      
                            main (ns)   HEAD (ns)   change                                                                                                                     
    0010                       142.93      137.68      -4%        
    0100                       512.12      305.18     -40%
    1000                      3004.60     1267.32     -58%                                                                                                                     
   
bench_extend_flatten                                                                                                                                                         
                            main (ns)   HEAD (ns)   change        
    0001_0010                  108.39      115.08      +6%
    0010_0100                  838.90      534.83     -36%                                                                                                                     
    0100_0100                 6745.96     1984.14     -71%
    1000_0010                 6196.60     1655.14     -73%                                                                                                                     

[the8472]

Vec code is very hot and highly responsive to perturbations, let's see what perf says

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: dc931d4 (dc931d446f7059c1a67b165a4c202d6107484ba5, parent: 64a965e9013a9d14e83c4d370af26f6be6bf96fb)

[rust-timer]

Error occured while categorizing benchmark run:

could not find start commit for bound Commit("64a965e9013a9d14e83c4d370af26f6be6bf96fb")

[mati865]

@rust-timer queue

[mati865]

@rust-timer build dc931d4

[rust-timer]

Failed to enqueue some commit SHAs. Maybe they were already benchmarked?

[mati865]

Scratch that. Let's start over.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8902743 (8902743fec7a53b48dd90e2ff86517f69bd1fe0f, parent: 800892799d7666fe1dc17abd862100a6cf273718)

[rust-timer]

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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	7.6%	[0.2%, 274.5%]	46
Regressions ❌ (secondary)	0.9%	[0.1%, 4.0%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.0%]	16
All ❌✅ (primary)	7.6%	[0.2%, 274.5%]	46

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.

	mean	range	count
Regressions ❌ (primary)	3.6%	[0.8%, 14.2%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-2.5%	[-2.6%, -2.3%]	2
All ❌✅ (primary)	2.8%	[-2.7%, 14.2%]	8

Cycles

Results (primary 16.7%, secondary 3.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	16.7%	[1.4%, 243.0%]	18
Regressions ❌ (secondary)	3.7%	[2.2%, 5.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	16.7%	[1.4%, 243.0%]	18

Binary size

Results (primary 1.2%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.0%, 4.0%]	60
Regressions ❌ (secondary)	0.6%	[0.0%, 1.0%]	9
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[-0.3%, 4.0%]	62

Bootstrap: 507.838s -> 524.419s (3.27%)
Artifact size: 398.10 MiB -> 402.39 MiB (1.08%)

[joboet]

Also see #138752 for a prior attempt at doing this.

View changes since this review

[joboet]

Vec::push_within_capacity does the same and avoids some unsafe.

[hkBst]

@joboet #138752 shows a similar +260% performance loss. Any idea what causes that?

[Mark-Simulacrum]

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.