Turn HIR indexing into a query #59064
Conversation
rust-highfive
added
the
S-waiting-on-review
src/librustc/dep_graph/dep_node.rs
Outdated
| @@ -458,6 +458,7 @@ define_dep_nodes!( <'tcx> | |||
| [eval_always] PrivacyAccessLevels(CrateNum), | |||
| [eval_always] CheckPrivateInPublic(CrateNum), | |||
| [eval_always] Analysis(CrateNum), | |||
| [eval_always] HirMap(CrateNum), | |||
There was a problem hiding this comment.
Probably need a true eval_always query type, since these are actually eval if HIR changes, and we don't know if the HIR changes until after the HIR is hashed, which happens in the HirMap query.
There was a problem hiding this comment.
This refers to #59091, right?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Zoxc
changed the title
|
@bors try |
Turn HIR indexing into a query r? @michaelwoerister
|
☀️ Try build successful - checks-travis |
|
@rust-timer build eb9092d |
|
Success: Queued eb9092d with parent 70d1150, comparison URL. |
|
Finished benchmarking try commit eb9092d |
|
☔ The latest upstream changes (presumably #56462) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
query-hir
branch
2 times, most recently
from
ddfc6c3
to
25b731e
Compare
src/librustc/ty/query/plumbing.rs
Outdated
| @@ -601,9 +601,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
| pub(super) fn ensure_query<Q: QueryDescription<'gcx>>(self, key: Q::Key) -> () { | |||
| let dep_node = Q::to_dep_node(self, &key); | |||
|
|
|||
| // Ensuring an "input" or anonymous query makes no sense | |||
| if dep_node.kind.is_eval_always() { | |||
There was a problem hiding this comment.
Why does ensuring an eval_always query make sense? No data should be cached by doing this, so at best we see some diagnostics?
There was a problem hiding this comment.
Ensuring queries just make sure the side effects of the query has been applied. This also applies to eval_always queries. However it doesn't make sense trying to mark them green with try_mark_green_and_read because we don't record dependencies for them, so they are handled separately here.
src/librustc/ty/context.rs
Outdated
|
|
||
| pub hir_defs: hir::map::Definitions, | ||
|
|
||
| hir_map: AtomicCell<Option<&'tcx hir_map::Map<'tcx>>>, |
src/librustc/ty/context.rs
Outdated
| @@ -1082,7 +1089,17 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
|
|
|||
| #[inline(always)] | |||
| pub fn hir(self) -> &'a hir_map::Map<'gcx> { | |||
| &self.hir_map | |||
| let value = self.hir_map.load(); | |||
| if unlikely!(value.is_none()) { | |||
There was a problem hiding this comment.
This should be simplified significantly by the use of Once
There was a problem hiding this comment.
Once is slow, and the hir function is very hot.
There was a problem hiding this comment.
ok, can you at least fold this into your AtomicCell (by passing a closure to some method) so the non-query code stays out of the query?
Since the AtomicCell is just used for this, maybe rename it to AtomicOnce and hide the Option from the use site.
There was a problem hiding this comment.
AtomicCell is a crossbeam-utils type. I can't modify it.
non-query code stays out of the query
Not sure what you mean here. This is just a regular function.
There was a problem hiding this comment.
I just saw your version in rustc_datastructures, but I realize now that's just for nonparallel rustc.
There was a problem hiding this comment.
Not sure what you mean here. This is just a regular function.
well it's like an emulated query. It would be a regular query just for the caching if it weren't so hot or had any arguments, right?
Which reminds me. Doesn't this being a memoization mean we'll not have an edge from queries calling hir to the hir_map query, except for the first query to call hir? Is that problematic?
There was a problem hiding this comment.
except for the first query to call
hir?
It uses with_ignore so there won't be any edges even for the first one. Having edges to the real hir_map query would be bad, as it is no_hash and eval_always which makes it effectively always red (and everything which depends on it always execute).
Not adding edges here is fine since the HIR map adds edges when you use it to access things.
There was a problem hiding this comment.
I ending up using this pattern in a couple more places so I'm adding a AtomicOnce type for it.
src/librustc/dep_graph/graph.rs
Outdated
| @@ -388,10 +388,7 @@ impl DepGraph { | |||
| |_| None, | |||
| |data, key, fingerprint, _| { | |||
| let mut current = data.borrow_mut(); | |||
| let krate_idx = current.node_to_node_index[ | |||
| &DepNode::new_no_params(DepKind::Krate) | |||
There was a problem hiding this comment.
eval_always queries are hardcoded to depend on the entire HIR (Krate) here. I changed these to instead always execute instead of executing only when the HIR changes. Also see comments / perf on #59091
|
☔ The latest upstream changes (presumably #59433) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
query-hir
branch
2 times, most recently
from
2db0fbe
to
df25514
Compare
|
☔ The latest upstream changes (presumably #60195) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We discussed this in the triage meeting. There were a few points raised:
So I'm removing nomination but in general we would like to hold up until we've had agreement on the overall "end-to-end" plan. |
|
☔ The latest upstream changes (presumably #60683) made this pull request unmergeable. Please resolve the merge conflicts. |
Mark-Simulacrum
added
S-waiting-on-team
|
Marking as waiting on the compiler team -- the overall plan needs to be sketched out and possibly a meeting scheduled to approve it for implementation. Once that happens, @Zoxc has a series of PRs which will need to be re-opened (#59404, #59338, #59282, and #59205) that depend on this one and each other. |
|
Has this been discussed at any point? |
|
triage meeting: we may want to dedicate a planning meeting to resolving the plan here |
|
☔ The latest upstream changes (presumably #61817) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
query-hir
branch
2 times, most recently
from
8a6d2a5
to
ebdd666
Compare
|
Your PR failed on Travis (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.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 |
|
Ping from triage |
|
@Zoxc any updates on this? Is there anything this is still blocked on? Thanks |
|
It's blocked on compiler team consensus. There is a meeting scheduled to talk about this. |
r? @michaelwoerister