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

Generate metadata by iterating on DefId instead of traversing the HIR tree #80347

Closed
wants to merge 16 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Dec 24, 2020

Most of the work in metadata encoding consists in walking the HIR, and calling the different queries on each node.
This can be done by simply looping on all the DefIds, and invoking each query directly.

This requires to create non-panicking versions of some queries.

Papercuts:

  • fix polymorphize tests;
  • find a better way to iterate on DefIds;
  • handle def_kind, entry_kind and children tables.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Dec 24, 2020
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 24, 2020

I like the idea, we definitely need to check perf and see how it fares, but I don't see actual problems with it.

I still think it needs an MCP, can you open one so the compiler team knows about it? We have a meeting later today, so it should go fast.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov self-assigned this Dec 24, 2020
@petrochenkov
Copy link
Contributor

This is an interesting idea, and there are multiple factors in play here

  • the code certainly looks cleaner
  • it's harder to forget to encode something like it happened in rustc_metadata: Do not forget to encode inherent impls for foreign types #77375
  • on the other hand it's easier to encode something dummy that otherwise wouldn't be encoded at all (this is a performance concern)
  • also HIR visiting should be very cheap compared to actual encoding so we may not win much here
  • if property X only exists for a small portion of DefIds then we may spend a lot of time querying it for all DefIds unnecessarily (also a performance concern)
  • on the other hand the query system can potentially support bulk queries ("give me Xs for all DefIds") instead of "for def_id in all_def_ids { give me X for def_id }" if the former has performance benefits

@petrochenkov petrochenkov removed their assignment Dec 24, 2020
@jyn514 jyn514 added A-metadata Area: Crate metadata T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 24, 2020
@cjgillot
Copy link
Contributor Author

* on the other hand it's easier to encode something dummy that otherwise wouldn't be encoded at all (this is a performance concern)
* if property X only exists for a small portion of `DefId`s then we may spend a lot of time querying it for all `DefId`s unnecessarily (also a performance concern)

I am also worried about this. This will definitely need a perf run.

* also HIR visiting should be very cheap compared to actual encoding so we may not win much here

I would like to drop the HIR earlier in the future. If this ever happens, HIR visiting will be impossible at such a late stage.

* on the other hand the query system can potentially support bulk queries ("give me Xs for all `DefId`s") instead of "for def_id in all_def_ids { give me X for def_id }" if the former has performance benefits

In the initial implementation, I wanted to use the query caches directly, like this:

tcx.queries.def_span.iter_results(|iter| {
  for (k, _, v) in iter {
    if !k.is_local() { continue }
    record!(self.tables.def_span[k] <- v);
  }
}

However, this did not force the queries if the def_id had not been forced earlier.
Since the forcing has to be done at some point, the proposed solution seemed to me the most robust.

@cjgillot
Copy link
Contributor Author

@oli-obk
Copy link
Contributor

oli-obk commented Dec 26, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Dec 26, 2020

⌛ Trying commit 5cdfe091c3363720dcd399d458ff4b317bf6f066 with merge 2e325a37b8ff6a4cce0abcacff6dc1b64a3cf624...

@bors
Copy link
Contributor

bors commented Dec 26, 2020

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

@rust-timer
Copy link
Collaborator

Queued 2e325a37b8ff6a4cce0abcacff6dc1b64a3cf624 with parent 30a4273, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 26, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2e325a37b8ff6a4cce0abcacff6dc1b64a3cf624): 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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 26, 2020
@bjorn3
Copy link
Member

bjorn3 commented Dec 26, 2020

Significant regressions of up to 50%. This is due to optimized_mir being invoked several times as much.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 27, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Dec 27, 2020

⌛ Trying commit 7ad4bee45bbafeae6ea1350d427bd22610062ae2 with merge 7ea7454bf3e5d01fd6f85df9620da4191ef48eaa...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2021
Iterate on DefId for variances and generics.

Split from rust-lang#80347
@bors
Copy link
Contributor

bors commented Mar 24, 2021

☔ The latest upstream changes (presumably #75384) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726 jackh726 assigned jackh726 and unassigned oli-obk Mar 25, 2021
@crlf0710
Copy link
Member

@cjgillot Ping from triage! There's merge conflicts now.

@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 4, 2021
@bstrie bstrie 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2021
@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

@cjgillot What are the next steps here? Should I review this PR as-is? Or do you want to continue splitting this into separate PRs?

@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2021

@cjgillot Ping from triage, any updates on this?

@bstrie bstrie 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 21, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2021
@camelid
Copy link
Member

camelid commented Aug 29, 2021

triage: @cjgillot What's the status of this? Does this PR need to be split up or do the merge conflicts just need to be fixed?

@cjgillot cjgillot closed this Aug 29, 2021
@cjgillot cjgillot deleted the defkey branch September 21, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metadata Area: Crate metadata S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.