Skip to content

Use Vec extend and collect instead of repeatedly calling push #90813

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

notriddle
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Contributor

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2021
@notriddle notriddle force-pushed the notriddle/vec-extend branch from bd28d5e to a82692d Compare November 11, 2021 23:11
@GuillaumeGomez
Copy link
Member

Nice! Since it'll very likely impact performance, I'll prevent putting it into a rollup.

@bors: r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Nov 12, 2021

📌 Commit a82692d has been approved by GuillaumeGomez

@bors 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 12, 2021
hellow554 added a commit to hellow554/rust that referenced this pull request Nov 12, 2021
…uillaumeGomez

Use Vec extend and collect instead of repeatedly calling push
@bors
Copy link
Collaborator

bors commented Nov 12, 2021

⌛ Testing commit a82692d with merge f31622a...

@bors
Copy link
Collaborator

bors commented Nov 12, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing f31622a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 12, 2021
@bors bors merged commit f31622a into rust-lang:master Nov 12, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 12, 2021
@notriddle notriddle deleted the notriddle/vec-extend branch November 12, 2021 15:34
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f31622a): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

  • Very large improvement in instruction counts (up to -8.7% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@GuillaumeGomez
Copy link
Member

Wow, that's much better than what I expected!

@notriddle
Copy link
Contributor Author

That’s the magic of the TrustedLen trait 😉

@camelid
Copy link
Member

camelid commented Nov 12, 2021

Wow, that is really impressive!

@camelid
Copy link
Member

camelid commented Nov 12, 2021

Since it'll very likely impact performance, I'll prevent putting it into a rollup.

Just a note: it's better to use rollup=never if a PR will very likely affect perf. Otherwise, you can't tell what the perf changes were, and it may mask perf regressions in other rolled-up PRs.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Nov 12, 2021

iffy means "better not put it into a rollup unless it's strongly needed for whatever reason". So seems like a good match.

@camelid
Copy link
Member

camelid commented Nov 12, 2021

iffy means "better not put it into a rollup unless it's strongly needed for whatever reason". So seems like a good match.

If for some reason, someone really needed to put it into a rollup, they could just mark the PR as rollup-able. In my experience, iffy usually means "prone to failures during bors testing" or "may have slight perf changes, but unlikely to be anything significant".

But it's not a big deal; it just makes perf tracking easier.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2021
Use Vec extend instead of repeated pushes on several places

Inspired by rust-lang#90813, I tried to use a simple regex (`for .*in.*\{\n.*push\(.*\);\n\s+}`) to search for more places that would use `Vec::push` in a loop and replace them with `Vec::extend`.

These probably won't have as much perf. impact as the original PR (if any), but it would probably be better to do a perf run to see if there are not any regressions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2021
Use Vec extend instead of repeated pushes on several places

Inspired by rust-lang#90813, I tried to use a simple regex (`for .*in.*\{\n.*push\(.*\);\n\s+}`) to search for more places that would use `Vec::push` in a loop and replace them with `Vec::extend`.

These probably won't have as much perf. impact as the original PR (if any), but it would probably be better to do a perf run to see if there are not any regressions.
bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request Dec 20, 2021
10484: internal: Update match checking algorithm r=lnicola a=iDawer

Sync match checking algorithm with rust-lang/rust f31622a50 2021-11-12 (rust-lang/rust#90813)

This update brings huge simplification to the match checking and introduces an easy to use machinery for pattern destructuring and also:

1. Add a function to do post-inference normalization `hir_ty::infer::normalize(...)`.
2. Store binding modes in `InferenceResult`.

Todo:

- [x] Rebase & test (#10484 (comment))

Co-authored-by: Dawer <7803845+iDawer@users.noreply.github.com>
Co-authored-by: iDawer <ilnur.iskhakov.oss@outlook.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants