Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Use spare_capacity_mut instead of invalid unchecked indexing when joining str #91680

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Use spare_capacity_mut instead of invalid unchecked indexing when joining str #91680

merged 1 commit into from
Dec 14, 2021

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 8, 2021

This is a fix for #91574

I think in general I'd prefer to see this code implemented with raw pointers or MaybeUninit::write_slice, but there's existing code in here based on copying from slice to slice, so converting everything from &[T] to &[MaybeUninit<T>] is less disruptive.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 Dec 8, 2021
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021
dtolnay
dtolnay approved these changes Dec 10, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dtolnay
Copy link
Member

dtolnay commented Dec 10, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit 14f4ffa has been approved by dtolnay

@bors
Copy link
Contributor

bors commented Dec 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Dec 10, 2021
@dtolnay dtolnay assigned dtolnay and unassigned yaahc Dec 10, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
@matthiaskrgr
Rollup merge of rust-lang#91680 - saethlin:spare_capacity_mut-in-join…
f33ca93 
…, r=dtolnay

Use spare_capacity_mut instead of invalid unchecked indexing when joining str

This is a fix for rust-lang#91574

I think in general I'd prefer to see this code implemented with raw pointers or `MaybeUninit::write_slice`, but there's existing code in here based on copying from slice to slice, so converting everything from `&[T]` to `&[MaybeUninit<T>]` is less disruptive.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Closed
@matthiaskrgr
Copy link
Member

@bors rollup=never might be perf sensitive

@bors
Copy link
Contributor

bors commented Dec 14, 2021

⌛ Testing commit 14f4ffa with merge 7ca74ea...

@bors
Copy link
Contributor

bors commented Dec 14, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 7ca74ea to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2021
@bors bors merged commit 7ca74ea into rust-lang:master Dec 14, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7ca74ea): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 5.4% on incr-unchanged builds of inflate)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 14, 2021
@pnkfelix
Copy link
Member

This looks like just inflate flagging something again as it bounces up and down during different benchmark runs. It jumps back down again in #91766 (the successor PR). I plan to remove both PR #91680 and #91766 from this weeks' triage report.

@rustbot label: +perf-regression-triaged.

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 15, 2021
@saethlin saethlin mentioned this pull request Dec 19, 2021
Closed
@saethlin saethlin deleted the spare_capacity_mut-in-join branch May 16, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.59.0
Development

Successfully merging this pull request may close these issues.
10 participants
@saethlin @rust-highfive @dtolnay @bors @matthiaskrgr @rust-timer @pnkfelix @yaahc @apiraino @rustbot