Add invariant to VecDeque::pop_* that len < cap if pop successful by Philippe-Cholet · Pull Request #123089 · rust-lang/rust

Philippe-Cholet · 2024-03-26T10:40:42Z

Similar to #114370 for VecDeque instead of Vec.

I initially come from rust-itertools/itertools#899 where we noticed that pop_front;push_back; was slower than expected so @scottmcm suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where kornel mentionned #114334 (fixed by #114370).

This is my first time with codegen tests, I based the test on what was done for Vec.

rustbot · 2024-03-26T10:40:50Z

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

Philippe-Cholet · 2024-03-26T11:45:23Z

I just saw that the Vec::pop optimization was updated to core::hint::assert_unchecked(self.len < self.capacity()); so I probably should too.

Noratrieb · 2024-03-26T14:00:20Z

Yeah, that's nicer.
Gonna check compile time performance in the meantime (i don't expect many results), don't push the change until the try build is done (~2h) or it will be cancelled
@bors try @rust-timer queue

…cap, r=<try> Add invariant to VecDeque::pop_* that len < cap if pop successful Similar to rust-lang#114370 for VecDeque instead of Vec. I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370). This is my first time with codegen tests, I based the test on what was done for Vec.

bors · 2024-03-26T14:01:33Z

⌛ Trying commit e822937 with merge debc5ab...

bors · 2024-03-26T15:39:41Z

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

rust-timer · 2024-03-26T17:37:16Z

Finished benchmarking commit (debc5ab): comparison URL.

Overall result: no relevant changes - 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-perf -perf-regression

Instruction count

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

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)	2.6%	[0.5%, 7.4%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.6%	[-13.3%, -0.5%]	5
Improvements ✅ (secondary)	-2.2%	[-2.9%, -1.7%]	3
All ❌✅ (primary)	-0.5%	[-13.3%, 7.4%]	10

Cycles

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)	0.8%	[0.4%, 1.3%]	2
Regressions ❌ (secondary)	2.2%	[2.0%, 2.3%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.4%, 1.3%]	2

Binary size

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)	0.1%	[0.0%, 0.3%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.5%, 0.3%]	7

Bootstrap: 672.189s -> 671.042s (-0.17%)
Artifact size: 315.72 MiB -> 315.75 MiB (0.01%)

Noratrieb · 2024-03-27T19:35:42Z

No regression, not that I expected any change.
@bors r+ rollup=maybe

bors · 2024-03-27T19:35:45Z

📌 Commit 083ee83 has been approved by Nilstrieb

It is now in the queue for this repository.

…e_cap, r=Nilstrieb Add invariant to VecDeque::pop_* that len < cap if pop successful Similar to rust-lang#114370 for VecDeque instead of Vec. I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370). This is my first time with codegen tests, I based the test on what was done for Vec.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#121943 (Clarify atomic bit validity) - rust-lang#123089 (Add invariant to VecDeque::pop_* that len < cap if pop successful) - rust-lang#123101 (Delegation: fix ICE on wrong `Self` instantiation) - rust-lang#123130 (Load missing type of impl associated constant from trait definition) - rust-lang#123133 (chore: fix some comments) - rust-lang#123136 (Some wording improvement) - rust-lang#123139 (`num::NonZero::get` can be 1 transmute instead of 2) - rust-lang#123142 (Let nils know about changes to target docs) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-03-27T22:28:28Z

@bors r-
#123143 (comment)

Similar to #114370 for VecDeque instead of Vec. It now uses `core::hint::assert_unchecked`.

Noratrieb · 2024-04-08T11:18:12Z

+
+#[no_mangle]
+// CHECK-LABEL: @push_back_byte(
+pub fn push_back_byte(v: &mut VecDeque<u8>) {


thanks for including those tests btw, having the test to make sure the above tests are correct is super important!

I merely mimicked the Vec version of this PR.
The 1st iteration was too much Vec-oriented.

Noratrieb · 2024-04-08T11:18:25Z

@bors r+

bors · 2024-04-08T11:18:27Z

📌 Commit 7a2678d has been approved by Nilstrieb

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#115984 (extending filesystem support for Hermit) - rust-lang#120144 (privacy: Stabilize lint `unnameable_types`) - rust-lang#122807 (Add consistency with phrases "meantime" and "mean time") - rust-lang#123089 (Add invariant to VecDeque::pop_* that len < cap if pop successful) - rust-lang#123595 (Documentation fix) - rust-lang#123625 (Stop exporting `TypeckRootCtxt` and `FnCtxt`.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123089 - Philippe-Cholet:vecdeque_pop_assume_cap, r=Nilstrieb Add invariant to VecDeque::pop_* that len < cap if pop successful Similar to rust-lang#114370 for VecDeque instead of Vec. I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370). This is my first time with codegen tests, I based the test on what was done for Vec.

rustbot assigned Noratrieb Mar 26, 2024

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 Mar 26, 2024

This comment has been minimized.
Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 26, 2024

This comment has been minimized.
Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 26, 2024

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 Mar 27, 2024

matthiaskrgr mentioned this pull request Mar 27, 2024
Rollup of 8 pull requests #123143
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 Mar 27, 2024

Add invariant to VecDeque::pop_* that len < cap if pop successful
7a2678d
Similar to #114370 for VecDeque instead of Vec. It now uses `core::hint::assert_unchecked`.

Noratrieb reviewed Apr 8, 2024
View reviewed changes

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 Apr 8, 2024

matthiaskrgr mentioned this pull request Apr 8, 2024
Rollup of 6 pull requests #123628
Merged

bors merged commit 17c94b5 into rust-lang:masterApr 8, 2024

rustbot added this to the 1.79.0 milestone Apr 8, 2024

Philippe-Cholet deleted the vecdeque_pop_assume_cap branch April 8, 2024 16:26

Uh oh!

Uh oh!

Conversation

Philippe-Cholet commented Mar 26, 2024

Uh oh!

rustbot commented Mar 26, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

Philippe-Cholet commented Mar 26, 2024

Uh oh!

Noratrieb commented Mar 26, 2024

Uh oh!

This comment has been minimized.

bors commented Mar 26, 2024

Uh oh!

bors commented Mar 26, 2024

Uh oh!

This comment has been minimized.

rust-timer commented Mar 26, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Noratrieb commented Mar 27, 2024

Uh oh!

bors commented Mar 27, 2024

Uh oh!

matthiaskrgr commented Mar 27, 2024

Uh oh!

NoratriebApr 8, 2024

Choose a reason for hiding this comment

Uh oh!

Philippe-CholetApr 8, 2024

Choose a reason for hiding this comment

Uh oh!

Noratrieb commented Apr 8, 2024

Uh oh!

bors commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants