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.

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

add slice::swap_unchecked #88540

Merged
merged 6 commits into from
Oct 15, 2021
Merged

add slice::swap_unchecked #88540

merged 6 commits into from
Oct 15, 2021

Conversation

ibraheemdev
Copy link
Member

An unsafe version of slice::swap that does not do bounds checking.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Aug 31, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@leonardo-m
Copy link

See #71874

@leonardo-m
Copy link

I think it's worth using this inside slice::reverse.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2021
@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 8, 2021
@camelid
Copy link
Member

camelid commented Oct 8, 2021

triage: @ibraheemdev could you address the above comments? Thanks!

@kennytm
Copy link
Member

kennytm commented Oct 14, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2021

📌 Commit cf12732 has been approved by kennytm

@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 Oct 14, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 15, 2021
add `slice::swap_unchecked`

An unsafe version of `slice::swap` that does not do bounds checking.
@matthiaskrgr
Copy link
Member

@bors rollup=never likely has some perf effects

@bors
Copy link
Contributor

bors commented Oct 15, 2021

⌛ Testing commit cf12732 with merge 1dafe6d...

@bors
Copy link
Contributor

bors commented Oct 15, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 1dafe6d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2021
@bors bors merged commit 1dafe6d into rust-lang:master Oct 15, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 15, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1dafe6d): comparison url.

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

  • Small regression in instruction counts (up to 1.5% on full builds of piston-image)

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 Oct 15, 2021
@kennytm
Copy link
Member

kennytm commented Oct 15, 2021

maybe revert the implementation of the safe swap, keeping swap and swap_unchecked separate? at least we don't need to do bounds checking 4 times in debug build.

@rylev
Copy link
Member

rylev commented Oct 19, 2021

@kennytm @ibraheemdev this seems to have landed with more bounds checking than is necessary though I would imagine that the extra checking shouldn't be happening in the compiler since debug assertions are off. While the regressions aren't huge, they are in real world crates and this is likely to effect the perf of many (most?) Rust programs in a negative way. Perhaps we should do the suggestion of reverting the implementation of the safe swap since it's not necessary for swap_unchecked to land.

Also FYI, when it's likely that a perf regression could come out of a change (as was pointed out) it's best to just run a perf run before merging.

@camelid
Copy link
Member

camelid commented Oct 19, 2021

While the regressions aren't huge, they are in real world crates and this is likely to effect the perf of many (most?) Rust programs in a negative way.

IIRC, std is always distributed with debug-assertions turned off (unless you build std yourself), so while it'd be good to fix the redundant bounds checks, I think it's unlikely to affect that many user crates.

I'm also surprised that there's a regression; like you said, I thought the compiler was by default built with debug assertions turned off. Maybe the bounds checks are not the source of the regression?

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2022
Revert implementation of `slice::swap`

Due to the perf regressions noticed here, possible due to inlining? rust-lang#88540 (comment)

r? `@kennytm`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.