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

Optimize character escaping. #124307

Merged
merged 3 commits into from
May 15, 2024
Merged

Optimize character escaping. #124307

merged 3 commits into from
May 15, 2024

Conversation

reitermarkus
Copy link
Contributor

Allow optimization of panicking branch in EscapeDebug, see #121805.

r? @joboet

@rustbot 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 Apr 23, 2024
This was referenced Apr 23, 2024
Closed
Open
@joboet
Copy link
Member

joboet commented Apr 23, 2024 •
edited
Loading

I'm so sorry, I had another look at this and found that my analysis was incorrect: I forgot to switch the compiler...

The actual cause (fingers crossed) is in EscapeIterInner::next (and the same applies to its next_back and as_ascii methods). The compiler does not know about this range invariant:

rust/library/core/src/escape.rs

Line 70 in 40dcd79

// Invariant: alive.start <= alive.end <= N.

so it cannot optimize out the branch checks here:

rust/library/core/src/escape.rs

Line 102 in 40dcd79

self.alive.next().map(|i| self.data[usize::from(i)].to_u8())

(Godbolt example)

The ufmt example iterates over an escaped EscapeDebug and therefore calls EscapeIterInner::next, leading to the panic branch.

You are welcome to fix this in this PR, but can also open another one and just assign me.

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Apr 25, 2024 •
edited
Loading

@joboet, I have now refactored EscapeIterInner to better contain the alive range invariant and changed the index operations to use get_unchecked.

joboet
joboet reviewed Apr 30, 2024
View reviewed changes
Copy link
Member

@joboet joboet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I love the approach, encapsulating the invariant is a very good idea. One small issue left.

I changed the PR title to make it fit the contents, feel free to adapt it further.

library/core/src/escape.rs Outdated Show resolved Hide resolved
@joboet joboet changed the title Inline EscapeDebug::size_hint. Optimize character escaping Apr 30, 2024
@joboet joboet 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-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2024
@reitermarkus
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2024
@reitermarkus
Copy link
Contributor Author

reitermarkus commented May 1, 2024 •
edited
Loading

@joboet, I now reverted to using Range<u8> since that version generates fewer instructions, and also added a hint when converting char to u32 which allows optimizing the index operations.

@reitermarkus reitermarkus requested a review from joboet May 1, 2024 15:57
@joboet joboet 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-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2024
@reitermarkus reitermarkus force-pushed the escape-debug-size-hint-inline branch from 46c9a9a to ec1775d Compare May 8, 2024 20:04
@reitermarkus reitermarkus changed the title Optimize character escaping Optimize character escaping. May 8, 2024
@reitermarkus reitermarkus mentioned this pull request May 8, 2024
Merged
@reitermarkus reitermarkus force-pushed the escape-debug-size-hint-inline branch from ec1775d to 57c84a5 Compare May 8, 2024 20:08
@reitermarkus reitermarkus requested a review from joboet May 8, 2024 20:23
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2024
@reitermarkus
Copy link
Contributor Author

@rustbot ready

joboet
joboet approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@joboet joboet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great, I just have two small nits left.

library/core/src/char/mod.rs Outdated Show resolved Hide resolved
library/core/src/char/mod.rs Outdated Show resolved Hide resolved
library/core/src/ascii.rs Outdated Show resolved Hide resolved
@reitermarkus reitermarkus force-pushed the escape-debug-size-hint-inline branch from 57c84a5 to 4edf12d Compare May 9, 2024 15:04
@reitermarkus reitermarkus requested a review from joboet May 9, 2024 15:14
@joboet
Copy link
Member

joboet commented May 15, 2024

Cool stuff, thank you!
@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2024

📌 Commit 4edf12d has been approved by joboet

It is now in the queue for this repository.

@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 May 15, 2024
@fmease fmease mentioned this pull request May 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
@bors
Auto merge of rust-lang#125144 - fmease:rollup-4uft293, r=fmease
ade234d 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#124307 (Optimize character escaping.)
 - rust-lang#124975 (Use an helper to move the files)
 - rust-lang#125027 (Migrate `run-make/c-link-to-rust-staticlib` to `rmake`)
 - rust-lang#125038 (Invert comparison in `uN::checked_sub`)
 - rust-lang#125104 (Migrate `run-make/no-cdylib-as-rdylib` to `rmake`)
 - rust-lang#125137 (MIR operators: clarify Shl/Shr handling of negative offsets)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3873a74 into rust-lang:master May 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 15, 2024
@bors
Copy link
Contributor

bors commented May 15, 2024

⌛ Testing commit 4edf12d with merge ade234d...

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
@rust-timer
Unrolled build for rust-lang#124307
8c4b3c7 
Rollup merge of rust-lang#124307 - reitermarkus:escape-debug-size-hint-inline, r=joboet

Optimize character escaping.

Allow optimization of panicking branch in `EscapeDebug`, see rust-lang#121805.

r? `@joboet`
@fmease
Copy link
Member

fmease commented May 15, 2024 •
edited
Loading

Perf regression. See #125144 (comment)

@joboet joboet mentioned this pull request May 20, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
@bors
Auto merge of rust-lang#125317 - joboet:optimize_escape, r=<try>
a2f180d 
Optimize `EscapeIterInner`

This optimizes `EscapeIterInner` by using `MaybeUninit` for unused array elements instead of initializing them with `ascii::Char::Null`.

Follow up to rust-lang#124307, CC `@reitermarkus`
@reitermarkus
Copy link
Contributor Author

escape_ascii produces 13 fewer instructions:

https://rust.godbolt.org/z/8eob57os8

escape_unicode produces the same number of instructions:

https://rust.godbolt.org/z/zv9f8eb1n

@reitermarkus reitermarkus mentioned this pull request May 20, 2024
Closed
@reitermarkus reitermarkus deleted the escape-debug-size-hint-inline branch May 24, 2024 20:02
celinval added a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
@github-actions @celinval @zhassan-aws
Automatic toolchain upgrade to nightly-2024-05-16 (rust-lang#3189)
695bdfc 
Update Rust toolchain from nightly-2024-05-15 to nightly-2024-05-16
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@8387315
up to
rust-lang@1871252.
The log for this commit range is:
rust-lang@1871252fc8 Auto merge of
rust-lang#125164 - fmease:rollup-s5vwzlg, r=fmease
rust-lang@734a109998 Rollup merge of
rust-lang#125159 - fmease:allow-unauth-labels-l-pg-z, r=jieyouxu
rust-lang@601e5d199f Rollup merge of
rust-lang#125154 - FractalFir:fnabi_doc, r=compiler-errors
rust-lang@09156291e5 Rollup merge of
rust-lang#125146 - Oneirical:panic-impl, r=jieyouxu
rust-lang@80f991e09b Rollup merge of
rust-lang#125142 - GuillaumeGomez:migrate-rustdoc-themes, r=jieyouxu
rust-lang@c5b17ec9d2 Rollup merge of
rust-lang#125003 - RalfJung:aligned_alloc, r=cuviper
rust-lang@257d222e4b Improved the
documentation of the FnAbi struct
rust-lang@72a48fc68c Allow
unauthenticated users to modify `L-*`, `PG-*` and `-Z*` labels
rust-lang@b21b74b5e6 Auto merge of
rust-lang#125134 - compiler-errors:negative-traits-are-not-notable, r=fmease
rust-lang@a7484d2e49 fix tidy
rust-lang@cae17ff42b rewrite
panic-impl-transitive
rust-lang@ade234d574 Auto merge of
rust-lang#125144 - fmease:rollup-4uft293, r=fmease
rust-lang@8d38f2fb11 Rollup merge of
rust-lang#125137 - RalfJung:mir-sh, r=scottmcm
rust-lang@2659ff3882 Rollup merge of
rust-lang#125104 - Oneirical:test6, r=jieyouxu
rust-lang@4f7d9d4ad8 Rollup merge of
rust-lang#125038 - ivan-shrimp:checked_sub, r=joboet
rust-lang@2804d4223b Rollup merge of
rust-lang#125027 - Oneirical:c-test-with-remove, r=jieyouxu
rust-lang@2e70bea168 Rollup merge of
rust-lang#124975 - lu-zero:move_file, r=clubby789
rust-lang@3873a74f8a Rollup merge of
rust-lang#124307 - reitermarkus:escape-debug-size-hint-inline, r=joboet
rust-lang@3cb0030fe9 Auto merge of
rust-lang#123413 - petrochenkov:delegmulti2, r=fmease
rust-lang@c765480efe Migrate
`run-make/rustdoc-themes` to new rmake
rust-lang@c87ae947eb Add new `htmldocck`
function to `run-make-support`
rust-lang@a71c3ffce9 Auto merge of
rust-lang#125032 - compiler-errors:crash-dump-dir, r=onur-ozkan
rust-lang@0afd50e852 MIR operators:
clarify Shl/Shr handling of negative offsets
rust-lang@44fa5fd39a Auto merge of
rust-lang#125136 - matthiaskrgr:rollup-ljm15m3, r=matthiaskrgr
rust-lang@5f1a120ee5 Rollup merge of
rust-lang#125135 - chenyukang:yukang-fix-116502, r=compiler-errors
rust-lang@f7c2934420 Rollup merge of
rust-lang#125132 - mejrs:diag, r=compiler-errors
rust-lang@a8ff937b07 Rollup merge of
rust-lang#125108 - Zalathar:info-bitmap-bytes, r=nnethercote
rust-lang@03ff673dcc Rollup merge of
rust-lang#124990 - fmease:expand-weak-aliases-within-cts, r=compiler-errors
rust-lang@75895f59b0 Fix the dedup error
because of spans from suggestion
rust-lang@9e7aff7945 Auto merge of
rust-lang#125031 - Oneirical:dynamic-libs, r=jieyouxu
rust-lang@8994840f7e rustdoc: Negative
impls are not notable
rust-lang@91a3f04a3f fix the test
rust-lang@0160bff4b1 Auto merge of
rust-lang#125084 - Jules-Bertholet:fix-125058, r=Nadrieril
rust-lang@c30b41012d delegation:
Implement list delegation
rust-lang@18d7411719 Add
`on_unimplemented" typo suggestions
rust-lang@81f7e54962 Port issue-11908 to
rmake
rust-lang@1f61cc3078 port
no-cdylib-as-rdylib test
rust-lang@b1e5e5161a remove cxx_flags
rust-lang@1f5837ae25 rewrite
c-link-to-rust-staticlib
rust-lang@5cc020d3df avoid using
aligned_alloc; posix_memalign is better-behaved
rust-lang@c81be68fb4 coverage: Remove
confusing comments from `CoverageKind`
rust-lang@bfadc3a9b9 coverage:
`CoverageIdsInfo::mcdc_bitmap_bytes` is never needed
rust-lang@fe8f66e4bc `rustc_hir_typeck`:
Account for `skipped_ref_pats` in `expr_use_visitor`
rust-lang@4db00fe229 Use an helper to
move the files
rust-lang@7fde7308bf reverse condition in
`uN::checked_sub`
rust-lang@848f3c2c6e Make crashes dump
mir to build dir
rust-lang@35a5be2833 Also expand weak
alias tys inside consts inside `expand_weak_alias_tys`
rust-lang@4edf12d33e Improve escape
methods.
rust-lang@16981ba406 Avoid panicking
branch in `EscapeIterInner`.
rust-lang@e3fc97be2b Inline
`EscapeDebug::size_hint`.

Co-authored-by: celinval <35149715+celinval@users.noreply.github.com>
Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 27, 2024
@bors
Auto merge of rust-lang#124905 - reitermarkus:u32-from-char-opt, r=sc…
3942254 
…ottmcm

Allow optimizing `u32::from::<char>`.

Extracted from rust-lang#124307.

This allows optimizing the panicking branch in the `escape_unicode` function, see https://rust.godbolt.org/z/61YhKrhvP.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jul 30, 2024
@bors
Auto merge of #124905 - reitermarkus:u32-from-char-opt, r=scottmcm
cf2d1a9 
Allow optimizing `u32::from::<char>`.

Extracted from rust-lang/rust#124307.

This allows optimizing the panicking branch in the `escape_unicode` function, see https://rust.godbolt.org/z/61YhKrhvP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joboet joboet Awaiting requested review from joboet

Assignees

@joboet joboet

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.80.0
Development

Successfully merging this pull request may close these issues.
5 participants
@reitermarkus @joboet @bors @fmease @rustbot