Skip to content

Put panic code path from copy_from_slice into cold function #74512

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

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

LukasKalbertodt
Copy link
Member

The previous assert_eq generated quite some code, which is especially problematic when this call is inlined. This commit also slightly improves the panic message from:

assertion failed: `(left == right)`
  left: `3`,
 right: `2`: destination and source slices have different lengths

...to:

source slice length (2) does not match destination slice length (3)

You can see the code bloat in assembly here.

@rust-highfive
Copy link
Contributor

r? @withoutboats

(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 Jul 19, 2020
@LukasKalbertodt LukasKalbertodt force-pushed the debloat-copy-from-slice branch 2 times, most recently from 7e6cbf3 to af4bf0d Compare July 19, 2020 12:20
@tesuji
Copy link
Contributor

tesuji commented Jul 19, 2020

The x86 asm is shorter: https://rust.godbolt.org/z/53aodf

@LukasKalbertodt LukasKalbertodt force-pushed the debloat-copy-from-slice branch from af4bf0d to a0615e6 Compare July 19, 2020 13:18
@bors
Copy link
Collaborator

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@LukasKalbertodt LukasKalbertodt force-pushed the debloat-copy-from-slice branch from a0615e6 to b044ea1 Compare July 28, 2020 15:50
@bors
Copy link
Collaborator

bors commented Aug 12, 2020

☔ The latest upstream changes (presumably #75066) made this pull request unmergeable. Please resolve the merge conflicts.

The previous `assert_eq` generated quite some code, which is especially
problematic when this call is inlined. This commit also slightly
improves the panic message from:

  assertion failed: `(left == right)`
    left: `3`,
   right: `2`: destination and source slices have different lengths

...to:

  source slice length (2) does not match destination slice length (3)
@LukasKalbertodt LukasKalbertodt force-pushed the debloat-copy-from-slice branch from b044ea1 to db99f98 Compare August 12, 2020 19:17
@LukasKalbertodt
Copy link
Member Author

Rebased.

r? @KodrAus

@KodrAus
Copy link
Contributor

KodrAus commented Aug 12, 2020

Nice! @bors r+

@bors
Copy link
Collaborator

bors commented Aug 12, 2020

📌 Commit db99f98 has been approved by KodrAus

@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 Aug 12, 2020
@bors
Copy link
Collaborator

bors commented Aug 12, 2020

⌛ Testing commit db99f98 with merge 847ba83...

@bors
Copy link
Collaborator

bors commented Aug 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: KodrAus
Pushing 847ba83 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 13, 2020
@bors bors merged commit 847ba83 into rust-lang:master Aug 13, 2020
@LukasKalbertodt LukasKalbertodt deleted the debloat-copy-from-slice branch August 13, 2020 07:19
@ecstatic-morse
Copy link
Contributor

Final perf results are here. This was a small win across the board, mostly concentrated in incremental builds. Nice work!

@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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. 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.

8 participants