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

Serialize span hygiene data #72121

Merged
merged 9 commits into from
Jul 27, 2020
Merged

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 11, 2020

Fixes #68686
Fixes #70963

This PR serializies global hygiene data into both the incremental compilation cache and the crate metadata. This allows hygiene information to be preserved across compilation sessions (both incremental and cross-crate).

When serializing a SyntaxContext, we simply write out the raw id from the current compilation session. Whenever we deserialize a SyntaxContext, we 'remap' the id to a fresh id in our current compilation session, and load the associated SyntaxContextData.

As a result, some 'upstream' SyntaxContextData will end up getting duplicated in 'downstream' crates. This only happens when we actually need to use an 'upstream' SyntaxContext, which occurs when we deserialize a Span that requires it.

We serialize an ExpnData into the metadata of the crate which generated it. An ExpnId is serialized as a reference into the crate which 'owns' the corresponding ExpnData, which avoids duplication in downstream crates.

I've included a macros 2.0 test which requires hygiene serialization to compile successfully.

TODO:

  • Determine how many additional DefIds we end up creating for ExpnIds - this may be significant for libcore, which uses macros heavily. Alternatively, we could try to compute a DefPathHash without making a corresponding DefId - however, this might significantly complicate the implementation. (We no longer create DefIds)
  • Investigate the overhead of duplicating SyntaxContextData in crate metadata.
  • Investigate how resolve_crate_root behaves with deserialized hygiene data - the current logic may be wrong.
  • Add additional tests. The effects of this PR are usually only noticeable when working with headache-inducing macro expansions (e.g. macros expanding to macros), so there are lots of corner cases to test.
  • Determine what to do about this:

let expansion = ExpnId::root(); // FIXME(jseyfried) intercrate hygiene

  • Determine if we need to do anything here - I think the fact that src/test/ui/hygiene/cross_crate_hygiene.rs passes means that this is working.

module.for_each_child(self.r, |this, ident, ns, binding| {
// Filter away ambiguous imports and anything that has def-site
// hygiene.
// FIXME: Implement actual cross-crate hygiene.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2020
@Aaron1011
Copy link
Member Author

r? @petrochenkov

@Aaron1011
Copy link
Member Author

Let's see if there are any obvious performance issues with this approach:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 11, 2020

⌛ Trying commit fa6ef57e1a2fa04dbf737d1307d82e402cc1f0ff with merge 8bdad37a5ef4b604e078804ddecf3ee24bc50748...

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 12, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8bdad37a5ef4b604e078804ddecf3ee24bc50748 (8bdad37a5ef4b604e078804ddecf3ee24bc50748)

@rust-timer
Copy link
Collaborator

Queued 8bdad37a5ef4b604e078804ddecf3ee24bc50748 with parent 99cb9cc, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 8bdad37a5ef4b604e078804ddecf3ee24bc50748, comparison URL.

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors

This comment has been minimized.

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 12, 2020

⌛ Trying commit 899f82d06559b5e53ab584b9579a3aa3a1a06811 with merge 0d27819f009a7737f26dc558d8350176ce8c5b07...

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 13, 2020

⌛ Trying commit 2679248592393888eb579e9495a726d9525b6028 with merge 6326e282a07844c03667b78cc4904c3e45b1a423...

@bors
Copy link
Contributor

bors commented May 13, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6326e282a07844c03667b78cc4904c3e45b1a423 (6326e282a07844c03667b78cc4904c3e45b1a423)

@rust-timer
Copy link
Collaborator

Queued 6326e282a07844c03667b78cc4904c3e45b1a423 with parent 769d12e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 6326e282a07844c03667b78cc4904c3e45b1a423, comparison URL.

@Aaron1011
Copy link
Member Author

This is going to require more work to reduce the performance impact. It may be necessary to re-visit assigning DefIds to each ExpnId.

@bors
Copy link
Contributor

bors commented Jul 27, 2020

⌛ Testing commit f7235a8 with merge 4d20beecfc50d9db8ae1c80347207215e6b2ca10...

@Dylan-DPC-zz
Copy link

@bors retry yield

@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
##[group]Run exit 1
exit 1
shell: /bin/bash --noprofile --norc -e -o pipefail {0}
##[endgroup]
##[error]Process completed with exit code 1.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jul 27, 2020

⌛ Testing commit f7235a8 with merge 2b1adaeca4d2cd3de0a743769cf724499bda5eda...

@Dylan-DPC-zz
Copy link

@bors retry

@bors
Copy link
Contributor

bors commented Jul 27, 2020

⌛ Testing commit f7235a8 with merge fa36f96...

@Aaron1011
Copy link
Member Author

@Dylan-DPC: I think this PR and #73265 shouldn't conflict.

@Dylan-DPC-zz
Copy link

I think so as well but it's more as a precatuionary measure. that pr has been trying to get merged for a while now so don't want to add more latency.

@bors
Copy link
Contributor

bors commented Jul 27, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing fa36f96 to master...

@nnethercote
Copy link
Contributor

This was a perf loss, as expected.

Aaron1011 added a commit to Aaron1011/solana that referenced this pull request Aug 1, 2020
Fixes solana-labs#10933

Now that rust-lang/rust#72121 has been merged,
using a `$crate` path from a nested `macro_rules!` will work properly
across multiple crates. This allows us to update to a nightly build
containing this PR, and stop using `::solana_sdk` to refer to the
`respan!` macro.
Aaron1011 added a commit to Aaron1011/solana that referenced this pull request Aug 4, 2020
Fixes solana-labs#10933

Now that rust-lang/rust#72121 has been merged,
using a `$crate` path from a nested `macro_rules!` will work properly
across multiple crates. This allows us to stop using `::solana_sdk`
to reference to the `respan!` macro.
@Aaron1011 Aaron1011 mentioned this pull request Aug 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2020
Aaron1011 added a commit to Aaron1011/solana that referenced this pull request Aug 12, 2020
This nightly build includes both rust-lang/rust#72121 and a working
`rustfmt`.
Aaron1011 added a commit to Aaron1011/solana that referenced this pull request Aug 18, 2020
Fixes solana-labs#10933

Now that rust-lang/rust#72121 has been merged,
using a `$crate` path from a nested `macro_rules!` will work properly
across multiple crates. This allows us to stop using `::solana_sdk`
to reference to the `respan!` macro.
mergify bot pushed a commit to solana-labs/solana that referenced this pull request Aug 18, 2020
Fixes #10933

Now that rust-lang/rust#72121 has been merged,
using a `$crate` path from a nested `macro_rules!` will work properly
across multiple crates. This allows us to stop using `::solana_sdk`
to reference to the `respan!` macro.
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
See rust-lang/rust#72121.

The PR fixes some hygiene issues and thus exposes that the originating
expression comes from macros.
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
See rust-lang/rust#72121.

The PR fixes some hygiene issues and thus exposes that the originating
expression comes from macros.
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 19, 2020
See rust-lang/rust#72121.

The PR fixes some hygiene issues and thus exposes that the originating
expression comes from macros.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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