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

Improve string literal unescaping #94316

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Improve string literal unescaping #94316

merged 2 commits into from
Feb 25, 2022

Conversation

nnethercote
Copy link
Contributor

Some easy wins that affect a few popular crates.

r? @matklad

@nnethercote
Improve scan_escape.
37d9ea7 
`scan_escape` currently has a fast path (for when the first char isn't
'\\') and a slow path.

This commit changes `scan_escape` so it only handles the slow path, i.e.
the actual escaping code. The fast path is inlined into the two call
sites.

This change makes the code faster, because there is no function call
overhead on the fast path. (`scan_escape` is a big function and doesn't
get inlined.)

This change also improves readability, because it removes a bunch of
mode checks on the the fast paths.
@nnethercote
Inline a hot closure in from_lit_token.
44308dc 
The change looks big because `rustfmt` rearranges things, but the only
real change is the inlining annotation.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 24, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2022
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Trying commit 44308dc with merge 237a467d69c269f182947a1c90a981529f0e1538...

@nnethercote
Copy link
Contributor Author

nnethercote commented Feb 24, 2022 •
edited
Loading

This won't benefit the benchmarks in rustc-perf much, the benefits are in other crates. Here are the instruction count results I got locally for check builds on rustc-perf plus the following benchmarks identified as relevant by this analysis: pkcs8-0.9.0-pre, bitvec-1.0.1, der-0.6.0-pre.1, and snafu-0.7.0.

Benchmark & Profile Scenario % Change Significance Factor?
pkcs8 check incr-unchanged -7.26% 36.28x
bitvec check incr-unchanged -6.76% 33.82x
der check incr-unchanged -5.87% 29.34x
pkcs8 check full -5.14% 25.69x
pkcs8 check incr-full -4.00% 20.00x
der check full -2.77% 13.86x
der check incr-full -2.24% 11.21x
bitvec check full -2.17% 10.85x
snafu check full -1.96% 9.78x
bitvec check incr-full -1.93% 9.64x
snafu check incr-full -1.52% 7.60x
snafu check incr-unchanged -1.34% 6.70x
coercions check incr-unchanged -1.01% 5.05x
coercions check incr-patched: println -0.90% 4.48x
coercions check full -0.49% 2.46x
coercions check incr-patched: add static arr item -0.42% 2.08x
coercions check incr-full -0.40% 2.00x
stm32f4 check incr-unchanged -0.35% 1.75x

@nnethercote
Copy link
Contributor Author

nnethercote commented Feb 24, 2022 •
edited
Loading

As for the relative importance of the two commits, here are check full instruction counts according to Cachegrind for pkcs8, which was my primary benchmark.

Original  190.1M
Commit 1  186.4M
Commit 2  180.0M

@petrochenkov petrochenkov self-assigned this Feb 24, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

☀️ Try build successful - checks-actions
Build commit: 237a467d69c269f182947a1c90a981529f0e1538 (237a467d69c269f182947a1c90a981529f0e1538)

@rust-timer
Copy link
Collaborator

Queued 237a467d69c269f182947a1c90a981529f0e1538 with parent e780264, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (237a467d69c269f182947a1c90a981529f0e1538): comparison url.

Summary: This benchmark run shows 5 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -0.7%
  • Largest improvement in instruction counts: -0.9% on incr-unchanged builds of externs debug

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2022
@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2022
@petrochenkov
Copy link
Contributor

r? @petrochenkov @bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2022

📌 Commit 44308dc has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 24, 2022
@petrochenkov
Copy link
Contributor

I'm going to return this to rollup-able state, we see the result, it's localized to 2-3 crates and should be visible in a rollup, no need to merge this separately.
@bors rollup=maybe

@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2022
@bors
Copy link
Contributor

bors commented Feb 24, 2022

⌛ Testing commit 44308dc with merge 6ac6b249df629dad06f915a563bc7073ef206bf6...

@bors
Copy link
Contributor

bors commented Feb 24, 2022

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 24, 2022
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2022
@petrochenkov
Copy link
Contributor

Network failures when accessing crates.io
@bors retry

@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 Feb 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94316 - nnethercote:improve-string-literal-…
25cc094 
…unescaping, r=petrochenkov

Improve string literal unescaping

Some easy wins that affect a few popular crates.

r? `@matklad`
@Dylan-DPC Dylan-DPC mentioned this pull request Feb 24, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94316 - nnethercote:improve-string-literal-…
5ad185a 
…unescaping, r=petrochenkov

Improve string literal unescaping

Some easy wins that affect a few popular crates.

r? ``@matklad``
This was referenced Feb 24, 2022
Closed
Merged
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2022
@bors
Auto merge of rust-lang#94333 - Dylan-DPC:rollup-7yxtywp, r=Dylan-DPC
4e82f35 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#91795 (resolve/metadata: Stop encoding macros as reexports)
 - rust-lang#93714 (better ObligationCause for normalization errors in `can_type_implement_copy`)
 - rust-lang#94175 (Improve `--check-cfg` implementation)
 - rust-lang#94212 (Stop manually SIMDing in `swap_nonoverlapping`)
 - rust-lang#94242 (properly handle fat pointers to uninhabitable types)
 - rust-lang#94308 (Normalize main return type during mono item collection & codegen)
 - rust-lang#94315 (update auto trait lint for `PhantomData`)
 - rust-lang#94316 (Improve string literal unescaping)
 - rust-lang#94327 (Avoid emitting full macro body into JSON errors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ec44d48 into rust-lang:master Feb 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 25, 2022
@nnethercote nnethercote deleted the improve-string-literal-unescaping branch February 25, 2022 00:51
@nnethercote
Copy link
Contributor Author

Note: several of the affected crates, such as pkcs8, der, and bitvec, all use include_str! to include big chunks of documentation from .md files into their code. This seems to be the reason why the string literal processing is hot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov approved these changes

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rust-timer @bors @petrochenkov @rust-log-analyzer @matklad @rust-highfive @rustbot