3

Add invariant to VecDeque::pop_* that len < cap if pop successful by Philippe...

 3 weeks ago
source link: https://github.com/rust-lang/rust/pull/123089
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

Conversation

Contributor

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.

kornelski reacted with thumbs up emojiriking reacted with hooray emoji

Collaborator

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

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.

This comment has been minimized.

Contributor

Author

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

Member

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

This comment has been minimized.

Contributor

⌛ Trying commit e822937 with merge debc5ab...

Contributor

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

This comment has been minimized.

Collaborator

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

Cycles

Results

Binary size

Results

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

Member

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

Contributor

📌 Commit 083ee83 has been approved by Nilstrieb

It is now in the queue for this repository.

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

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

#[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!

Contributor

Author

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

Nilstrieb reacted with thumbs up emoji

Member

@bors r+

Contributor

📌 Commit 7a2678d has been approved by Nilstrieb

It is now in the queue for this repository.

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

bors

merged commit 17c94b5 into

rust-lang:master

Apr 8, 2024

11 checks passed

rustbot

added this to the 1.79.0 milestone

Apr 8, 2024

rust-timer

added a commit to rust-lang-ci/rust that referenced this pull request

Apr 8, 2024

Philippe-Cholet

deleted the vecdeque_pop_assume_cap branch

April 8, 2024 16:26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Nilstrieb

Nilstrieb left review comments
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects

None yet

Milestone

1.79.0

Development

Successfully merging this pull request may close these issues.

None yet

7 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK