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.

Sign up for GitHub

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

Jump to bottom

Move HIR parenting information out of hir_owner #83114

Merged
merged 6 commits into from
May 1, 2021
Merged

Move HIR parenting information out of hir_owner #83114

merged 6 commits into from
May 1, 2021

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Mar 14, 2021 •
edited
Loading

Split out of #82681.

The parent of a HIR node and its content are currently bundled together, but are rarely used together.
This PR separates both information in two distinct queries for HIR owners.
This reduces incremental invalidation for HIR items that appear within a function body when this body (and the local ids) changes.

@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2021
@bors
Copy link
Contributor

bors commented Mar 14, 2021

⌛ Trying commit c7c7d09b0353f3497678ac8ec32cad918b81f837 with merge 2f4b1b626e8b2095543ef80fa81b59c78514040c...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued 2f4b1b626e8b2095543ef80fa81b59c78514040c with parent 84c08f8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2f4b1b626e8b2095543ef80fa81b59c78514040c): 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 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 14, 2021
@cjgillot
Copy link
Contributor Author

rustbot did not assign anybody.
r? @petrochenkov

@petrochenkov
Copy link
Contributor

I think it's better to r? @michaelwoerister on this.
Also cc @eddyb who had opinions on HIR parenting, IIRC.

@bors
Copy link
Contributor

bors commented Mar 17, 2021

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

eddyb
eddyb reviewed Mar 18, 2021
View reviewed changes
compiler/rustc_middle/src/query/mod.rs
Comment on lines +55 to +61
/// Gives access to the HIR node's parent for the HIR owner `key`.
///
/// This can be conveniently accessed by methods on `tcx.hir()`.
/// Avoid calling this query directly.
query hir_owner_parent(key: LocalDefId) -> hir::HirId {
eval_always
desc { |tcx| "HIR parent of `{}`", tcx.def_path_str(key.to_def_id()) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, I think, if it's not a significant performance loss.

@cjgillot cjgillot mentioned this pull request Mar 20, 2021
Closed
@bors
Copy link
Contributor

bors commented Mar 25, 2021

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

@bors
Copy link
Contributor

bors commented Mar 29, 2021

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

@cjgillot cjgillot mentioned this pull request Mar 31, 2021
Merged
@bors
Copy link
Contributor

bors commented Apr 29, 2021

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

eddyb
eddyb approved these changes Apr 30, 2021
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me (not 100% on the incremental aspects, but it looks like the bulk of the changes account for the reshuffling of the data representation, and shouldn't change behavior outside of increasing incremental precision)

@varkor
Copy link
Member

varkor commented May 1, 2021

@bors r=eddyb

@bors
Copy link
Contributor

bors commented May 1, 2021

📌 Commit d794cb0 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 May 1, 2021
@Aaron1011
Copy link
Member

It looks like this is a regression on several benchmarks - is there anything that can be done about that?

@bors
Copy link
Contributor

bors commented May 1, 2021

⌛ Testing commit d794cb0 with merge 6e2a344...

@bors
Copy link
Contributor

bors commented May 1, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2021
@bors bors merged commit 6e2a344 into rust-lang:master May 1, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 1, 2021
This was referenced May 1, 2021
Merged
Merged
@cjgillot cjgillot deleted the hop branch May 1, 2021 22:16
@eddyb
Copy link
Member

eddyb commented May 3, 2021

It looks like this is a regression on several benchmarks - is there anything that can be done about that?

Oh sheesh, I completely missed that, sorry. Maybe nominate for discussion? I don't have a strong opinion, I just didn't see it.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2021
@bors
Auto merge of rust-lang#83723 - cjgillot:ownernode, r=petrochenkov
6489ee1 
Store all HIR owners in the same container

This replaces the previous storage in a BTreeMap for each of Item/ImplItem/TraitItem/ForeignItem.
This should allow for a more compact storage.

Based on rust-lang#83114
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jul 29, 2021
@bors
Auto merge of #83723 - cjgillot:ownernode, r=petrochenkov
1270ed0 
Store all HIR owners in the same container

This replaces the previous storage in a BTreeMap for each of Item/ImplItem/TraitItem/ForeignItem.
This should allow for a more compact storage.

Based on rust-lang/rust#83114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@michaelwoerister michaelwoerister

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
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.