Fix incremental bugs in the HIR map #69015
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Feb 10, 2020
[do not merge] Fix incremental bugs in the HIR map
|
The job 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 |
|
☀️ Try build successful - checks-azure |
|
Queued f84d986 with parent 840bdc3, future comparison URL. |
|
Finished benchmarking try commit f84d986, comparison URL. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit e510684e6fbca72455b335482cf6538dd20dc0f2 with merge f2aa56cac4a0eb137d35d91ec60e476122a752e5... |
|
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.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 |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-author
|
@bors try |
|
⌛ Trying commit e510684e6fbca72455b335482cf6538dd20dc0f2 with merge dcc5ac1c7dd14bb31920c83fef238a462e9ec511... |
|
The job 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 |
|
☀️ Try build successful - checks-azure |
|
Queued dcc5ac1c7dd14bb31920c83fef238a462e9ec511 with parent b6690a8, future comparison URL. |
|
Finished benchmarking try commit dcc5ac1c7dd14bb31920c83fef238a462e9ec511, comparison URL. |
|
I added a |
Zoxc
changed the title
|
Are these perf results still current? |
|
Yeah. I just removed a commit and its reversion. |
| } else { | ||
| DepKind::HirBody | ||
| }; | ||
| self.dep_graph.read(def_path_hash.to_dep_node(kind)); |
There was a problem hiding this comment.
I don't understand what this logic is supposed to do exactly (before or after the change)...
There was a problem hiding this comment.
It is supposed to add a read to the node which reveals the parent of the HirId.
There was a problem hiding this comment.
OK, I understand the original thinking now: The fingerprint of HirBody contains the information of the entire item (including the "signature" parts) so adding a dependency to HirBody is the conservative choice.
But as you discovered the parent is not hashed into any of the two dep-nodes, so we are clearly missing dependencies here.
I assume that the if hir_id.local_id == ItemLocalId::from_u32_const(0) check is based on the assumption that the parent of any node except the item root is part of the same HIR item and thus the HirBody fingerprint contains the appropriate information? If that's the case it is far from obvious and a clarifying comment will be much appreciated by anybody trying to understand this code.
| @@ -46,9 +47,17 @@ impl<'tcx> TyCtxt<'tcx> { | |||
| pub fn hir(self) -> Hir<'tcx> { | |||
| Hir { tcx: self, map: &self.hir_map } | |||
| } | |||
|
|
|||
| pub fn hir_id_parent_module(self, id: HirId) -> DefId { | |||
There was a problem hiding this comment.
Maybe parent_hir_module would be more appropriate.
There was a problem hiding this comment.
Maybe renaming the query to parent_module_from_def_id and letting this take parent_module is a better option?
| } | ||
|
|
||
| pub fn provide(providers: &mut Providers<'_>) { | ||
| providers.parent_module = |tcx, id| { | ||
| let hir = tcx.hir(); | ||
| hir.get_module_parent(hir.as_local_hir_id(id).unwrap()) |
There was a problem hiding this comment.
Can we make this function get_module_parent and get_module_parent_node less public? The query is always the better choice, right?
|
The fix looks good to me. Am I correct in assuming the #68944 will make the whole thing obsolete? |
|
@michaelwoerister #68944 includes the |
bors
added a commit
that referenced
this pull request
Mar 1, 2020
Use a query to get parent modules Split out from #69015 / #68944. r? @michaelwoerister
|
☔ The latest upstream changes (presumably #69380) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? @pnkfelix |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.