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

resolve: Collapse macro_rules scope chains on the fly #78826

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

petrochenkov
Copy link
Contributor

Otherwise they grow too long and you have to endlessly walk through them when resolving macros or imports.
Addresses https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Slow.20Builtin.20Derives/near/215750815

@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Nov 6, 2020
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 6, 2020

⌛ Trying commit 9221079 with merge 2f473715c794d9062bd0439b02d25f098de6acb5...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

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

@rust-timer
Copy link
Collaborator

Queued 2f473715c794d9062bd0439b02d25f098de6acb5 with parent a601302, future comparison URL.

@jyn514 jyn514 added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2f473715c794d9062bd0439b02d25f098de6acb5): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@eddyb
Copy link
Member

eddyb commented Nov 7, 2020

Looking over the implementation, I believe it is similar to the "path compression" optimization for union-find (collapsing redirects in a chain, i.e. replacing A -> B -> C -> D with A -> D where B and C are semantically noops, mere "aliases").

Here the chain is that of lexical (can only be seen "below" the definition, like let) macro_rules! names (as opposed to macro, which works like other Rust definitions), and the "noops" entries in the chain come from "there is a macro invocation which could expand to macro_rules! definitions" entries that got expanded (and don't themselves define macro_rules! macros).

Unlike union-find, however, this optimization is done eagerly (on every expansion), as opposed to lazily (when traversing these chains). I'm trusting @petrochenkov that this is more efficient, even though it might be clearer as an optimization to lazily cache-in-place during traversal, the way union-find "path compression" tends to work. Either way, I trust I can:

@bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2020

📌 Commit 9221079 has been approved by eddyb

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

@eddyb
I'll try using lazy compression in a follow up.
I'm not sure what would be more efficient, on one hand I suspect that all these scopes are going to be traversed in the end, on another hand the eager update like it's done now can result in multiple updates where lazy one could result in one.

@bors
Copy link
Contributor

bors commented Nov 9, 2020

⌛ Testing commit 9221079 with merge 4cf4348857446540c82e56139dc70301730d7dae...

@bors
Copy link
Contributor

bors commented Nov 9, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2020
@petrochenkov
Copy link
Contributor Author

#78665 - spurious, but known to fail often.
Let's @bors retry anyway.

@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 Nov 9, 2020
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 12, 2020

Network error (rust-lang/cargo#8517).

@bors retry

@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 Nov 12, 2020
@bors
Copy link
Contributor

bors commented Nov 12, 2020

⌛ Testing commit 9221079 with merge 50589431d4b535b1edc7925f9d726652c213c3e5...

@bors
Copy link
Contributor

bors commented Nov 12, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 12, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 12, 2020

Same error :/ @ehuss mentioned in rust-lang/cargo#8517 (comment) that this happens more during GitHub outages and YouTube went down for a bit around an hour ago, so maybe that's the issue? Going to avoid retrying for now.

@ehuss
Copy link
Contributor

ehuss commented Nov 12, 2020

It's not related to outages, it's just a case of bad luck. The pack files in the git repositories are in flux as they are constantly updated (each time a crate is published) and periodically recompressed. If it happens to recompress the pack file in such a way that it hits an edge condition, then it will fail. The only options are to run again and hope that the load balancer picks a different server that packed it differently, or to wait for the affected server to rebuild its pack files (which seems to happen every hour or two? I'm not sure) and hope it doesn't hit the edge case again.

@kennytm
Copy link
Member

kennytm commented Nov 12, 2020

@bors retry

A PR has been successfully merged, assuming the error has been fixed.

@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 Nov 12, 2020
@bors
Copy link
Contributor

bors commented Nov 12, 2020

⌛ Testing commit 9221079 with merge 4eb07ca60aff0364d21f2be1d080dce86e3b2ec5...

@bors
Copy link
Contributor

bors commented Nov 12, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 12, 2020
@petrochenkov
Copy link
Contributor Author

warning: spurious network error (1 tries remaining): error inflating zlib stream; class=Zlib (5)

@bors retry

@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 Nov 12, 2020
@bors
Copy link
Contributor

bors commented Nov 13, 2020

⌛ Testing commit 9221079 with merge a38f8fb...

@bors
Copy link
Contributor

bors commented Nov 13, 2020

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing a38f8fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 13, 2020
@bors bors merged commit a38f8fb into rust-lang:master Nov 13, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) I-compiletime Issue: Problems and improvements with respect to compile times. 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
Development

Successfully merging this pull request may close these issues.