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

Avoid hygiene_data lookups #61253

Merged
merged 5 commits into from
May 30, 2019
Merged

Avoid hygiene_data lookups #61253

merged 5 commits into from
May 30, 2019

Conversation

nnethercote
Copy link
Contributor

These commits mostly introduce compound operations that allow two close adjacent hygiene_data lookups to be combined.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2019
@nnethercote
Copy link
Contributor Author

Local results were very good, let's hope the CI results are similar. Instruction counts:

packed-simd-check
        avg: -6.4%      min: -10.2%     max: -0.5%
helloworld-check
        avg: -2.4%      min: -2.8%      max: -2.2%
unify-linearly-check
        avg: -1.9%      min: -2.7%      max: -1.3%
deeply-nested-check
        avg: -1.6%      min: -2.7%      max: -1.1%
script-servo-check
        avg: -2.0%      min: -2.6%      max: -1.6%
webrender-check
        avg: -1.8%      min: -2.4%      max: -1.2%
cranelift-codegen-check
        avg: -1.8%      min: -2.3%      max: -1.4%
style-servo-check
        avg: -1.8%      min: -2.1%      max: -1.5%
issue-46449-check
        avg: -1.5%      min: -1.9%      max: -1.3%
inflate-check
        avg: -0.5%      min: -1.6%      max: -0.1%
crates.io-check
        avg: -1.1%      min: -1.5%      max: -0.6%
regression-31157-check
        avg: -1.0%      min: -1.5%      max: -0.6%
sentry-cli-check
        avg: -1.2%      min: -1.5%      max: -0.9%
regex-check
        avg: -1.1%      min: -1.4%      max: -1.0%
tokio-webpush-simple-check
        avg: -1.0%      min: -1.4%      max: -0.7%
ripgrep-check
        avg: -1.0%      min: -1.2%      max: -0.8%
wg-grammar-check
        avg: -0.6%      min: -1.2%      max: -0.2%
futures-check
        avg: -0.7%      min: -1.1%      max: -0.4%
serde-check
        avg: -0.7%      min: -1.0%      max: -0.5%
piston-image-check
        avg: -0.8%      min: -1.0%      max: -0.6%
encoding-check
        avg: -0.8%      min: -1.0%      max: -0.5%
syn-check
        avg: -0.7%      min: -1.0%      max: -0.5%
clap-rs-check
        avg: -0.6%      min: -0.9%      max: -0.5%
hyper-check
        avg: -0.7%      min: -0.9%      max: -0.5%
unused-warnings-check 
        avg: -0.6%      min: -0.8%      max: -0.5%

@bors try

@bors
Copy link
Contributor

bors commented May 28, 2019

⌛ Trying commit 265ab6448ef63300dd6b74206be2a3316bcd56e5 with merge 9f4ea3d6210445892cbd666b2413dcf95a68a363...

@nnethercote
Copy link
Contributor Author

@Manishearth: the new outer_expn_info() function introduced in the second commit can be used in Clippy -- every single occurence of outer() in Clippy is immediately followed by expn_info(). (Note that some of these occurrences span two lines, and won't be found with a simple grep.) It's possible that using the new function might speed up Clippy.

@bors
Copy link
Contributor

bors commented May 28, 2019

☀️ Try build successful - checks-travis
Build commit: 9f4ea3d6210445892cbd666b2413dcf95a68a363

@Manishearth
Copy link
Member

Thanks for the heads up!

@nnethercote
Copy link
Contributor Author

@rust-timer build 9f4ea3d6210445892cbd666b2413dcf95a68a363

@rust-timer
Copy link
Collaborator

Success: Queued 9f4ea3d6210445892cbd666b2413dcf95a68a363 with parent b711179, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

19 similar comments
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea3d6210445892cbd666b2413dcf95a68a363, comparison URL.

@petrochenkov
Copy link
Contributor

Suggestion: make outer(), expn_info() and is_descendant_of() private methods on HygieneData and call them from hte Mark/SyntaxCtxt methods.

(Everything else looks good, but I didn't look at the last commit yet, will look later.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2019
@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 May 29, 2019
@nnethercote
Optimize TyCtxt::adjust_ident.
2232321 
It's a hot function that returns a 2-tuple, but the hottest call site
(`hygienic_eq`) discards the second element.

This commit renames `adjust_ident` as `adjust_ident_and_get_scope`, and
then introduces a new `adjust_ident` that only computes the first
element. This change also avoids the need to pass in an unused
`DUMMY_HIR_ID` argument in a couple of places, which is nice.
@nnethercote
Add HygieneData::{outer,expn_info,is_descendant_of} methods.
95ea7fd 
This commit factors out some repeated code.
@nnethercote
Copy link
Contributor Author

@petrochenkov: New code is up. I have addressed your comments. The new functions are in a new commit at the end of the series.

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2019

📌 Commit 95ea7fd 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2019
@Centril
Copy link
Contributor

Centril commented May 30, 2019

@bors rollup=never

for perf.

@Centril
Copy link
Contributor

Centril commented May 30, 2019

@bors p=1

because never.

@rust-lang rust-lang deleted a comment from bors May 30, 2019
@rust-lang rust-lang deleted a comment from rust-highfive May 30, 2019
@bors
Copy link
Contributor

bors commented May 30, 2019

⌛ Testing commit 95ea7fd with merge 0bfbaa6...

bors added a commit that referenced this pull request May 30, 2019
@bors
Auto merge of #61253 - nnethercote:avoid-hygiene_data-lookups, r=petr…
0bfbaa6 
…ochenkov

Avoid `hygiene_data` lookups

These commits mostly introduce compound operations that allow two close adjacent `hygiene_data` lookups to be combined.

r? @petrochenkov
@bors
Copy link
Contributor

bors commented May 30, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 0bfbaa6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2019
@bors bors merged commit 95ea7fd into rust-lang:master May 30, 2019
@nnethercote nnethercote deleted the avoid-hygiene_data-lookups branch May 30, 2019 20:21
@nnethercote
Copy link
Contributor Author

Final perf improvements are here.

@nnethercote nnethercote mentioned this pull request Jun 11, 2019
Closed
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nnethercote @bors @Manishearth @rust-timer @michaelwoerister @Mark-Simulacrum @petrochenkov @Centril @rust-highfive