`VecDeque::resize` should re-use the buffer in the passed-in element #104435

scottmcm · 2022-11-15T08:08:35Z

Today it always copies it for every appended element, but one of those clones is avoidable.

This adds iter::repeat_n (#104434) as the primitive needed to do this. If this PR is acceptable, I'll also use this in Vec rather than its custom ExtendElement type & infrastructure that is harder to share between multiple different containers:

rust/library/alloc/src/vec/mod.rs

Lines 2479 to 2492 in 101e182

    
           trait ExtendWith<T> { 
        
               fn next(&mut self) -> T; 
        
               fn last(self) -> T; 
        
           } 
        
           struct ExtendElement<T>(T); 
        
           impl<T: Clone> ExtendWith<T> for ExtendElement<T> { 
        
               fn next(&mut self) -> T { 
        
                   self.0.clone() 
        
               } 
        
               fn last(self) -> T { 
        
                   self.0 
        
               } 
        
           }

rustbot · 2022-11-15T08:08:41Z

r? @joshtriplett

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

rustbot · 2022-11-15T08:08:44Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

scottmcm · 2022-11-15T08:11:45Z

src/test/codegen/iter-repeat-n-trivial-drop.rs

+// CHECK-LABEL: @vec_extend_via_iter_repeat_n
+pub fn vec_extend_via_iter_repeat_n() -> Vec<u8> {
+    // CHECK: %[[ADDR:.+]] = tail call dereferenceable_or_null(1234) ptr @__rust_alloc(i64 1234, i64 1)
+    // CHECK: tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(1234) %[[ADDR]], i8 42, i64 1234,


This helps show that we can probably use the iterator rather than special custom extend code like

rust/library/alloc/src/vec/mod.rs

Lines 2525 to 2529 in 101e182

if n > 0 {

// We can write the last element directly without cloning needlessly

ptr::write(ptr, value.last());

local_len.increment_len(1);

}

but Vec is really important so I don't want to do that in the same PR as this fix.

scottmcm · 2022-11-15T08:14:14Z

Hopefully I can do this despite the new API -- with doc(hidden) if needed -- so I'll try a non-libs-api reviewer who hopefully is less busy than I know Josh is
r? @thomcc

Today it always copies it for *every* appended element, but one of those clones is avoidable.

thomcc · 2022-11-16T08:00:03Z

Do you want to open an ACP for the new API? I don't mind reviewing the impl, though.

scottmcm · 2022-11-16T08:00:58Z

ACP's already open: rust-lang/libs-team#120

thomcc · 2022-11-16T08:01:57Z

Thanks, I'll get to the review tomorrow or so then.

thomcc

This is a bit tricky but not very much code and it's pretty well commented, so doesn't seem that bad overall.

r=me on impl, I think it needs the ACP to be seconded to land (although I think the API is a good idea)

scottmcm · 2022-11-16T22:04:45Z

@thomcc To be able to decouple the VecDeque improvement from the ACP discussion, how would you feel about me marking the function & struct [doc(hidden)] for now? (Not pub(crate) so that the codegen tests can verify it and such.)

thomcc · 2022-11-18T20:26:21Z

After some discussion with other reviewers (for example, @cuviper), I think this is fine. We just need to remember to change the approach if the ACP is rejected, and it probably should be doc(hidden) initially.

scottmcm · 2022-11-19T04:31:16Z

@bors r=thomcc rollup

bors · 2022-11-19T04:31:18Z

📌 Commit 71bb200 has been approved by thomcc

It is now in the queue for this repository.

thomcc · 2022-11-19T05:21:49Z

Unsure about rollup here, shouldn't this be expected to improve perf if the compiler uses VecDeque::resize? (which it might not, but it's hard to say)

scottmcm · 2022-11-19T05:30:16Z

I'll just ask perf, then!

@bors r-

scottmcm · 2022-11-19T05:30:25Z

@bors try @rust-timer queue

scottmcm · 2022-11-19T05:44:26Z

@thomcc Given how big the bors queue has been lately, I'd rather do a perf run and rollup than make it run on its own, was my thought.

bors · 2022-11-19T07:36:08Z

☀️ Try build successful - checks-actions
Build commit: ba6c2e262132dbff0c22d04a0c095a8ff5643767 (ba6c2e262132dbff0c22d04a0c095a8ff5643767)

rust-timer · 2022-11-19T08:55:38Z

Finished benchmarking commit (ba6c2e262132dbff0c22d04a0c095a8ff5643767): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.6%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	1.7%	[1.0%, 2.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-2.3%, -0.8%]	3
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Cycles

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

scottmcm · 2022-11-19T19:34:53Z

@bors r=thomcc rollup

bors · 2022-11-19T19:34:54Z

📌 Commit 71bb200 has been approved by thomcc

It is now in the queue for this repository.

scottmcm · 2022-11-19T19:36:45Z

Two secondary regressions in code that doesn't use VecDeque suggest to me that this isn't actually a meaningful change, and thus the rollup is fine.

Rollup of 6 pull requests Successful merges: - rust-lang#103901 (Add tracking issue for `const_arguments_as_str`) - rust-lang#104112 (rustdoc: Add copy to the description of repeat) - rust-lang#104435 (`VecDeque::resize` should re-use the buffer in the passed-in element) - rust-lang#104467 (Fix substraction with overflow in `wrong_number_of_generic_args.rs`) - rust-lang#104608 (Cleanup macro matching recovery) - rust-lang#104626 (Fix doctest errors related to rustc_middle) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

scottmcm · 2022-12-28T23:19:48Z

(The rollup had no perf changes, so this was probably neutral.)

rustbot assigned joshtriplett Nov 15, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 15, 2022

scottmcm commented Nov 15, 2022

View reviewed changes

rustbot assigned thomcc and unassigned joshtriplett Nov 15, 2022

scottmcm force-pushed the iter-repeat-n branch from e880bef to 4df448a Compare November 15, 2022 08:38

scottmcm mentioned this pull request Nov 15, 2022

Tracking Issue for iter::repeat_n() (feature(iter_repeat_n)) #104434

Open

5 tasks

VecDeque::resize should re-use the buffer in the passed-in element

d62b903

Today it always copies it for *every* appended element, but one of those clones is avoidable.

scottmcm force-pushed the iter-repeat-n branch from 4df448a to d62b903 Compare November 15, 2022 08:53

thomcc approved these changes Nov 16, 2022

View reviewed changes

Hide the items while waiting for the ACP

71bb200

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2022

scottmcm mentioned this pull request Nov 19, 2022

Simplify Vec using iter::repeat_n #104596

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 19, 2022

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 19, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2022

scottmcm mentioned this pull request Nov 20, 2022

Use normal iterators instead of the internal vec::ExtendWith #104624

Closed

JohnTitor mentioned this pull request Nov 20, 2022

Rollup of 6 pull requests #104629

Merged

bors merged commit 785237d into rust-lang:master Nov 20, 2022

rustbot added this to the 1.67.0 milestone Nov 20, 2022

scottmcm deleted the iter-repeat-n branch November 20, 2022 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`VecDeque::resize` should re-use the buffer in the passed-in element #104435

`VecDeque::resize` should re-use the buffer in the passed-in element #104435

scottmcm commented Nov 15, 2022

rustbot commented Nov 15, 2022

rustbot commented Nov 15, 2022

scottmcm Nov 15, 2022

scottmcm commented Nov 15, 2022

thomcc commented Nov 16, 2022

scottmcm commented Nov 16, 2022

thomcc commented Nov 16, 2022

thomcc left a comment

scottmcm commented Nov 16, 2022

thomcc commented Nov 18, 2022 •

edited

Loading

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

thomcc commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

This comment has been minimized.

rust-timer commented Nov 19, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Dec 28, 2022

	trait ExtendWith<T> {
	fn next(&mut self) -> T;
	fn last(self) -> T;
	}

	struct ExtendElement<T>(T);
	impl<T: Clone> ExtendWith<T> for ExtendElement<T> {
	fn next(&mut self) -> T {
	self.0.clone()
	}
	fn last(self) -> T {
	self.0
	}
	}

	if n > 0 {
	// We can write the last element directly without cloning needlessly
	ptr::write(ptr, value.last());
	local_len.increment_len(1);
	}

VecDeque::resize should re-use the buffer in the passed-in element #104435

VecDeque::resize should re-use the buffer in the passed-in element #104435

Conversation

scottmcm commented Nov 15, 2022

rustbot commented Nov 15, 2022

rustbot commented Nov 15, 2022

scottmcm Nov 15, 2022

Choose a reason for hiding this comment

scottmcm commented Nov 15, 2022

thomcc commented Nov 16, 2022

scottmcm commented Nov 16, 2022

thomcc commented Nov 16, 2022

thomcc left a comment

Choose a reason for hiding this comment

scottmcm commented Nov 16, 2022

thomcc commented Nov 18, 2022 • edited Loading

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

thomcc commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

This comment has been minimized.

rust-timer commented Nov 19, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

scottmcm commented Nov 19, 2022

bors commented Nov 19, 2022

scottmcm commented Nov 19, 2022

scottmcm commented Dec 28, 2022

`VecDeque::resize` should re-use the buffer in the passed-in element #104435

`VecDeque::resize` should re-use the buffer in the passed-in element #104435

thomcc commented Nov 18, 2022 •

edited

Loading