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

Inline some parser functions #94693

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Inline some parser functions #94693

merged 2 commits into from
Mar 22, 2022

Conversation

nnethercote
Copy link
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

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

@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 Mar 7, 2022
@bors
Copy link
Contributor

bors commented Mar 7, 2022

⌛ Trying commit b190b2221a849ea63821fb44f980f6d6bc3bd5cc with merge cdabcb0018ab49622ef6ecf1567eb2aee3bc19b0...

@nnethercote
Copy link
Contributor Author

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

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

@bors
Copy link
Contributor

bors commented Mar 7, 2022

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

@rust-timer
Copy link
Collaborator

Queued cdabcb0018ab49622ef6ecf1567eb2aee3bc19b0 with parent 3d1eaf4, future comparison URL.

@rust-timer
Copy link
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

@rustbot rustbot 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
Copy link
Contributor Author

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
Copy link
Contributor Author

r? @petrochenkov

@petrochenkov
Copy link
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
Copy link
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?

@petrochenkov petrochenkov 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
Copy link
Contributor Author

I have made the requested changes.

@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
Copy link
Contributor Author

I have updated again.

@petrochenkov petrochenkov 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
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2022

📌 Commit f8f1d3f has been approved by petrochenkov

@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 Mar 22, 2022
@bors
Copy link
Contributor

bors commented Mar 22, 2022

⌛ Testing commit f8f1d3f with merge 64137f0...

@bors bors mentioned this pull request Mar 22, 2022
Merged
@bors
Copy link
Contributor

bors commented Mar 22, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2022
@bors bors merged commit 64137f0 into rust-lang:master Mar 22, 2022
@rustbot rustbot 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
Copy link
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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. 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.
6 participants
@nnethercote @rust-timer @bors @petrochenkov @Aaron1011 @rustbot