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

Introduce LocalDefId to HirId lookup table #72552

Closed
wants to merge 1 commit into from
Closed

Introduce LocalDefId to HirId lookup table #72552

wants to merge 1 commit into from

Conversation

marmeladema
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2020
@marmeladema
Copy link
Contributor Author

If thats ok @ecstatic-morse , i'd like to run a perf run.

@ecstatic-morse
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 24, 2020

⌛ Trying commit 20f20ac7a07b7db98b26c50cf0926cc7a2f818db with merge ba85afd575ddccb47ea677511836c480c50c2b61...

@ecstatic-morse
Copy link
Contributor

BTW, you committed an .orig file by accident. I think you need to wait for the perf run to finish before pushing again though.

@ecstatic-morse
Copy link
Contributor

@marmeladema This is basically just #63849 right? @ljedrz did we come to a conclusion about the approach in #63849 (comment)?

@bors
Copy link
Contributor

bors commented May 25, 2020

☀️ Try build successful - checks-azure
Build commit: ba85afd575ddccb47ea677511836c480c50c2b61 (ba85afd575ddccb47ea677511836c480c50c2b61)

@rust-timer
Copy link
Collaborator

Queued ba85afd575ddccb47ea677511836c480c50c2b61 with parent 46e85b4, future comparison URL.

@petrochenkov petrochenkov self-assigned this May 25, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ba85afd575ddccb47ea677511836c480c50c2b61, comparison URL.

@ljedrz
Copy link
Contributor

ljedrz commented May 25, 2020

@ecstatic-morse I wasn't sure how to proceed there; there was a follow-up PR, #65837, but it was closed due to inactivity.

@marmeladema
Copy link
Contributor Author

Rebased and removed the mistakenly added .orig file.

About the approach here, I think the first commit is valuable no matter what because it just removes some useless uses of NodeId and final conversion in <clone|into>_outputs methods. Its just cleanup from #72402

The 2 following commits are about removing all uses of as_local_node_id which are not really needed and helps reduce the APIs involving NodeId.

And the last one is about replacing the now unused def_id_to_node_id internal table by a LocalDefId to HirId table in order to achieve direct conversion without having to use an intermediary table involving NodeIds.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 26, 2020 •
edited
Loading

I wasn't sure how to proceed there; there was a follow-up PR, #65837, but it was closed due to inactivity.

Ugh, that's a bummer.

So @nikomatsakis (who reviewed #63849) and @eddyb (who authored #65837). AFAICT with my somewhat limited understanding of the current HirId situation, this PR basically does the same thing as #63849. It replaces the current DefId -> NodeId -> HirId lookup chain with a direct mapping from DefId -> HirId to try to limit the use of NodeIds in the later stages of compilation. As @eddyb noted in #63849 (comment), this is somewhat silly, since a HirId is just a LocalDefId combined with an ItemLocalId, so we should be able to map to a HirId by using a "default" ItemLocalId (hopefully I'm not misinterpreting that too badly). #63849 was closed with the idea that @ljedrz would pursue this idea instead with mentorship from @eddyb.

@eddyb opened #65837 to get this process started, but it was never seriously reviewed, so things have sort of stalled out. I guess I see two options:

@marmeladema
Copy link
Contributor Author

@ecstatic-morse i think as a start, i can split out the cleanup commits in a separate PR in order to focus in the import bits. Should the cleanup include commit 1 or 1,2,3? Including the 3 commits would reduce the API surface involving NodeId

@marmeladema marmeladema mentioned this pull request May 26, 2020
Merged
@marmeladema
Copy link
Contributor Author

I've split out the first commit into its own PR and i'll do the same for the next 2 commits if needed.

@nikomatsakis
Copy link
Contributor

Splitting out the commits seems good -- I think the appropriate next course of action would be a MCP. IIRC, the older attempts pre-dated the system, and a big part of the problem (for me) was that I never understood what was being proposed, not really. =) Or at least I remember feeling confused. A summary like the one that @ecstatic-morse just gave is very helpful-- I would be in favor of trying to adopt the "correct" scheme, but I think it'd be worth actually describing what that is apart from its implementation.

@petrochenkov
Copy link
Contributor

Blocked on #72636.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 29, 2020
@Dylan-DPC
Rollup merge of rust-lang#72636 - marmeladema:resolver-outputs-def-id…
48ea37b 
…, r=petrochenkov

Cleanup `Resolver::<clone|into>_outputs` methods

Follow-up cleanup work of rust-lang#72402

First commit has been split out from rust-lang#72552

r? @ecstatic-morse
@bors

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 29, 2020
petrochenkov
petrochenkov reviewed May 29, 2020
View reviewed changes
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/build_reduced_graph.rs Outdated Show resolved Hide resolved
src/librustc_hir/definitions.rs
@@ -457,7 +443,7 @@ impl Definitions {
// Create the definition.
let def_id = LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) };

assert_eq!(self.def_id_to_node_id.push(node_id), def_id);
assert_eq!(self.def_id_to_hir_id.push(None), def_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we need these pushes with None if we re-populate the whole def_id_to_hir_id table below anyway.

@petrochenkov
Copy link
Contributor

The change looks entirely reasonably to me and it passes CI so it doesn't trip the HirId validation, so I'm not sure why it would need an MCP.

Also, the first two commits could also be landed separately if the third one causes some questions.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2020
@petrochenkov
Copy link
Contributor

Looks like we are not too far from being able to eliminate NodeIds entirely.

NodeIds are assigned in InvocationCollector and DefIds are assigned in DefCollector, which run immediately one after another during expansion.
So NodeIds as they exist now are only needed during a very short window, which perhaps can be eliminated eventually by merging passes or some other approach.

@marmeladema
Copy link
Contributor Author

For ease of review, i'll split out the first two commits in a separate pull request.

@marmeladema marmeladema mentioned this pull request May 29, 2020
Merged
@petrochenkov
Copy link
Contributor

Blocked on #72750.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 30, 2020
@JohnTitor
Rollup merge of rust-lang#72750 - marmeladema:remove-as-local-node-id…
c20a97d 
…, r=petrochenkov

Remove remaining calls to `as_local_node_id`

Split out from rust-lang#72552

cc rust-lang#50928
@bors
Copy link
Contributor

bors commented May 30, 2020

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

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 30, 2020
@petrochenkov petrochenkov removed their assignment May 30, 2020
@ecstatic-morse ecstatic-morse mentioned this pull request Jun 4, 2020
Merged
@marmeladema
Replace def_id_to_node_id table by a def_id_to_hir_table
67fcf63 
Since the removal of all calls to `as_local_node_id`, the `def_id_to_node_id`
table was useless. It is being replaced by a `def_id_to_hir_id` table
which allows to lookup `HirId` from `LocalDefId` without indirection
through `NodeId` tables.
@marmeladema
Copy link
Contributor Author

Rebased on master

@petrochenkov
Copy link
Contributor

@marmeladema
#73178 requires keeping the map "LocalDefId -> NodeId", but only until creation of HirIds.

The reason is that early lints have to work with NodeIds until HirIds are created.
(Local)DefIds are too coarse-grained for that task.

@petrochenkov
Copy link
Contributor

The ultimate solution to this problem is probably creating HirId earlier than AST -> HIR lowering, somewhere around creating DefIds.

Otherwise there's always some window in which you have no option except that to use NodeIds if you need a fine-grained id.

@marmeladema
Copy link
Contributor Author

I think keeping the map LocalDefId -> NodeId before the lowering is totally fine. I just think after hir lowering, all NodeIds should be gone for good. Ultimately i kind of think this map should belong to the resolver itself since its rely tied to the resolver job.

@petrochenkov
Copy link
Contributor

Ultimately i kind of think this map should belong to the resolver itself since its rely tied to the resolver job.

Yeah, it should be ok to move it from Definitions to Resolver.

@marmeladema
Copy link
Contributor Author

I've worked on another approach that might be better than this one actually. Let's close this for now.

@marmeladema marmeladema deleted the hir-id-ification branch April 24, 2021 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@ecstatic-morse ecstatic-morse

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@marmeladema @ecstatic-morse @rust-timer @bors @ljedrz @nikomatsakis @petrochenkov @rust-highfive