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.

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

Avoid more hygiene lookups #61484

Merged
merged 16 commits into from Jun 5, 2019

Conversation

@nnethercote
Copy link
Contributor

commented Jun 3, 2019

Mostly by combining multiple HygieneData::with calls into a single call on hot paths.

r? @petrochenkov

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

⌛️ Trying commit 485f834 with merge 9840af4...

bors added a commit that referenced this pull request Jun 3, 2019

@hellow554

This comment was marked as outdated.

Copy link
Contributor

commented Jun 3, 2019

Why all the XXX in the commit message? 😕 doesn't look nice

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

☀️ Try build successful - checks-travis
Build commit: 9840af4

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Jun 3, 2019

Success: Queued 9840af4 with parent c57ed9d, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Jun 3, 2019

Finished benchmarking try commit 9840af4, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Why all the XXX in the commit message? 😕 doesn't look nice

Because it's not finished. I filed the PR because I wanted to do a perf run on CI. That's why I did r? @ghost.

@nnethercote nnethercote force-pushed the nnethercote:avoid-more-hygiene-lookups branch from 485f834 to af026f6 Jun 4, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I fixed up the commit messages and edited the PR description above.

r? @petrochenkov

@nnethercote nnethercote force-pushed the nnethercote:avoid-more-hygiene-lookups branch from af026f6 to 1d50c5f Jun 4, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

At this point (after introduction of all these combined methods) perhaps it makes sense to introduce hygiene::get_data() returning a short-lived reference to HygieneData (LocalInternedString-style) and allow other crates to combine operations themselves.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

If HygieneData goes the same way as #59742, it will end up kept in Session with an optional TLS access when you really cannot pass the session.

hygiene::get_data() will be replaced with sess.hygiene_data in this case.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Ok, there are 4 "obviously combined" operations right now - outer_is_descendant_of, modernize_and_adjust, outer_expn_info, outer_and_expn_info - not too bad.
I think we can live like this until HygieneData is moved into Session.

r=me with nits addressed

nnethercote added some commits May 31, 2019

Add `HygieneData::default_transparency`.
Also use `HygieneData::expn_info` in an appropriate place.
Optimize `glob_adjust` and `reverse_glob_adjust`.
They can each now do a single `HygieneData::with` call by replacing the
`SyntaxContext` and `Mark` methods with the equivalent methods from
`HygieneData`.
Add `HygieneData::apply_mark_with_transparency`.
Also remove `HygieneData::apply_mark_internal`, which is no longer
needed.
Add `HygieneData::apply_mark`.
This combines two `HygieneData::with` calls into one.
Add and use `SyntaxContext::outer_and_expn_info`.
This combines two `HygieneData::with` calls into one on a hot path.

nnethercote added some commits Jun 2, 2019

Add `SyntaxContext::hygienic_eq`.
This combines multiple `HygieneData::with` calls into one, by combining
parts of `hygienic_eq` and `adjust_ident`.
Add `walk_chain`.
This combines multiple `HygieneData::with` calls on a hot path.
Avoid unnecessary `rust_2018` calls.
The commit combines two calls into one by saving the result in a local
variable. The commit also moves the check for `async` later, so that
when a different keyword is present the `rust_2018` call will be avoided
completely.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

At this point (after introduction of all these combined methods) perhaps it makes sense to introduce hygiene::get_data() returning a short-lived reference to HygieneData (LocalInternedString-style) and allow other crates to combine operations themselves.

Exposing HygieneData::with would be safer. LocalInternedString's handling of lifetimes is quite dubious and not something we want to copy elsewhere, IMO.

Add `modernize_and_adjust` methods.
These combine two `HygieneData::with` calls into one.

@nnethercote nnethercote force-pushed the nnethercote:avoid-more-hygiene-lookups branch from 1d50c5f to 4c9ecbf Jun 4, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I addressed the comments.

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

📌 Commit 4c9ecbf has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

⌛️ Testing commit 4c9ecbf with merge 2a1d6c8...

bors added a commit that referenced this pull request Jun 5, 2019

Auto merge of #61484 - nnethercote:avoid-more-hygiene-lookups, r=petr…
…ochenkov

Avoid more hygiene lookups

Mostly by combining multiple `HygieneData::with` calls into a single call on hot paths.

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

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

@bors bors added the merged-by-bors label Jun 5, 2019

@bors bors merged commit 4c9ecbf into rust-lang:master Jun 5, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:avoid-more-hygiene-lookups branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.