Skip to content

Inline some parser functions - #94693

Merged
bors merged 2 commits into
rust-lang:masterfrom
nnethercote:parser-inlining
Mar 22, 2022
Merged

Inline some parser functions#94693
bors merged 2 commits into
rust-lang:masterfrom
nnethercote:parser-inlining

Conversation

@nnethercote

@nnethercotennethercote commented Mar 7, 2022

Copy link
Copy Markdown
Contributor

Some crates that do a lot of complex declarative macro expansion spend a lot of time parsing (and reparsing) tokens. These commits inline some functions for some minor speed wins.

r? @ghost

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 7, 2022
@nnethercote

nnethercote commented Mar 7, 2022

Copy link
Copy Markdown
ContributorAuthor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Mar 7, 2022

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2022
@bors

bors commented Mar 7, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit b190b2221a849ea63821fb44f980f6d6bc3bd5cc with merge cdabcb0018ab49622ef6ecf1567eb2aee3bc19b0...

@nnethercote

nnethercote commented Mar 7, 2022

Copy link
Copy Markdown
ContributorAuthor

Here are some local instruction count measurements, including a number of crates that aren't in the usual benchmark suite.

Benchmark & ProfileScenario% ChangeSignificance Factor?
time-macros-0.2.3 checkfull-4.19%20.94x
async-std-1.10.0 checkfull-3.33%16.64x
yansi-0.5.0 checkfull-2.10%10.49x
num-derive-0.3.3 checkfull-1.44%7.18x
web-sys-0.3.56 checkfull-1.38%6.88x
futures-macro-0.3.19 checkfull-1.35%6.75x
ctor-0.1.21 checkfull-1.34%6.70x
mockall_derive-0.11.0 checkfull-1.20%5.98x
pest_generator-2.1.3 checkfull-1.19%5.93x
scroll_derive-0.11.0 checkfull-1.05%5.26x
deep-vector checkfull-0.84%4.20x
inotify-0.10.0 checkfull-0.80%3.99x
externs checkfull-0.49%2.47x
syn checkfull-0.41%2.06x

@bors

bors commented Mar 7, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Mar 7, 2022

Copy link
Copy Markdown
Collaborator

Queued cdabcb0018ab49622ef6ecf1567eb2aee3bc19b0 with parent 3d1eaf4, future comparison URL.

@rust-timer

rust-timer commented Mar 7, 2022

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cdabcb0018ab49622ef6ecf1567eb2aee3bc19b0): comparison url.

Summary: This benchmark run did not return any relevant results.

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

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 7, 2022
@nnethercote

nnethercote commented Mar 7, 2022

Copy link
Copy Markdown
ContributorAuthor

r? @Aaron1011

The CI run was meh, but I think the improvements on the benchmarks outside of rustc-perf are clear enough for this to be worth doing, given that the changes are simple.

@nnethercote

nnethercote commented Mar 18, 2022

Copy link
Copy Markdown
ContributorAuthor

r? @petrochenkov

@petrochenkov

petrochenkov commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

Can it use inlined #[inline(always)] fn inlined_foo() { ... } and regular fn foo() { inlined_foo() } pairs instead?

In the uninlined cases we don't really care whether it's inlined or not and can leave the decision to the compiler, but it would make the code cleaner, IMO.

@petrochenkov

petrochenkov commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

Could you also add short comments to the inlined versions telling that they are supposed to be used in especially hot contexts found by profiling?

@petrochenkovpetrochenkov 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 Mar 18, 2022
@nnethercote

nnethercote commented Mar 20, 2022

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes.

petrochenkov
petrochenkov reviewed Mar 20, 2022
View reviewed changes
Comment threadcompiler/rustc_parse/src/parser/mod.rs Outdated
@nnethercote
Split Parser::bump_with into inlined and non-inlined halves.
4e700a0
The call site within `Parser::bump` is hot.
Also add an inline annotation to `Parser::next_tok`. It was already
being inlined by the compiler; this just makes sure that continues.
@nnethercote
Split TokenCursor::{next,next_desugared} into inlined and non-inlin…
f8f1d3f
…ed halves.
@nnethercote

nnethercote commented Mar 22, 2022

Copy link
Copy Markdown
ContributorAuthor

I have updated again.

@petrochenkovpetrochenkov 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 Mar 22, 2022
@petrochenkov

petrochenkov commented Mar 22, 2022

Copy link
Copy Markdown
Contributor

Thanks!
@bors r+

@bors

bors commented Mar 22, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit f8f1d3f has been approved by petrochenkov

@borsbors 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 Mar 22, 2022
@bors

bors commented Mar 22, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit f8f1d3f with merge 64137f0...

@borsbors mentioned this pull request Mar 22, 2022
Merged
@bors

bors commented Mar 22, 2022

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 64137f0 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2022
@bors
bors merged commit 64137f0 into rust-lang:masterMar 22, 2022
@rustbotrustbot added this to the 1.61.0 milestone Mar 22, 2022
@nnethercote
nnethercote deleted the parser-inlining branch March 22, 2022 20:11
@rust-timer

rust-timer commented Mar 22, 2022

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (64137f0): comparison url.

Summary: This benchmark run did not return any relevant results.

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

@rustbot label: -perf-regression

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

Reviewers

@petrochenkovpetrochenkovpetrochenkov left review comments

Assignees

@petrochenkovpetrochenkov

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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.

6 participants

@nnethercote@rust-timer@bors@petrochenkov@Aaron1011@rustbot