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

[incremental] don't use NodeId in the HIR, but something local to an item #40303

Closed
nikomatsakis opened this issue Mar 6, 2017 · 6 comments
Closed
Labels
A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

The current NodeId design is hostile to incremental compilation because a change anywhere in the HIR affects numbering everywhere (particularly when desugarings are taken into account). @michaelwoerister encountered problems with metadata hashing being very unstable as a result; it also makes loading/unloading more painful.

At the compiler design sprint, @michaelwoerister and @eddyb hatched a plan to make all NodeId usage in the HIR be replaced with "local ids", that are relative to an enclosing item (or something like that). I'll let them describe it here.

@nikomatsakis nikomatsakis added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2017
@eddyb
Copy link
Member

eddyb commented Mar 6, 2017

I think @michaelwoerister started work on it, actually. If he can't get back to it, maybe there's a branch?
To avoid redoing potentially most of the work, I mean.

@michaelwoerister
Copy link
Member

@eddyb I'm back to work now and I'm looking into it. There are lots of little issues to be solved.

The general plan is to have a hir::NodeId:

struct NodeId {
    owner: DefIndex,
    local_id: LocalNodeId,
}

struct LocalNodeId(u32);

This hir::NodeId still completely identifies a piece of HIR within the current crate but it has the advantage of being stable with respect to changes in unrelated items. When the owner is known from the context, we can also only store the LocalNodeId to save space and relocation effort. One example of this (I presume) would be the keys in ty::TypeckTables.

@nrc, @jonathandturner: @eddyb indicated that save analysis data operates on ast::NodeId. Do you have any concerns if we moved the HIR to the above scheme?

@eddyb
Copy link
Member

eddyb commented Mar 7, 2017

I think the only thing you need is a mapping to be able to access HIR paths and the astconv cache.

@nrc
Copy link
Member

nrc commented Mar 7, 2017

indicated that save analysis data operates on ast::NodeId

we basically only use node ids for looking up type (and name resolution) info. As long as you update the save-analysis crate to use whatever replacement ids you implement to do this lookup (I assume we won't even compile if you don't) then we should be fine. Externally, save-analysis uses def ids for everything. I guess there might be some issues if we convert node ids to def ids for the local crate? I forget exactly how that works now.

@michaelwoerister
Copy link
Member

I guess there might be some issues if we convert node ids to def ids for the local crate?

Not every NodeId has a corresponding DefId, but if that were a problem, you would already run into it now, I think.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 20, 2017
Introduce HirId, a replacement for ast::NodeId after lowering to HIR

This is the first step towards implementing rust-lang#40303. This PR introduces the `HirId` type and generates a `HirId` for everything that would be assigned one (i.e. stuff in the HIR), but the HIR data types still use `NodeId` for now. Changing that is a big refactoring that I want to do in a separate PR.

A `HirId` uniquely identifies a node in the HIR of the current crate. It is composed of the `owner`, which is the `DefIndex` of the directly enclosing `hir::Item`, `hir::TraitItem`, or `hir::ImplItem` (i.e. the closest "item-like"), and the `local_id` which is unique within the given owner.

This PR is also running a number of consistency checks for the generated `HirId`s:
- Does `NodeId` in the HIR have a corresponding `HirId`?
- Is the `owner` part of each `HirId` consistent with its position in the HIR?
- Do the numerical values of the `local_id` part all lie within a dense range of integers?

cc @rust-lang/compiler

r? @eddyb or @nikomatsakis
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 23, 2017
Introduce HirId, a replacement for ast::NodeId after lowering to HIR

This is the first step towards implementing rust-lang#40303. This PR introduces the `HirId` type and generates a `HirId` for everything that would be assigned one (i.e. stuff in the HIR), but the HIR data types still use `NodeId` for now. Changing that is a big refactoring that I want to do in a separate PR.

A `HirId` uniquely identifies a node in the HIR of the current crate. It is composed of the `owner`, which is the `DefIndex` of the directly enclosing `hir::Item`, `hir::TraitItem`, or `hir::ImplItem` (i.e. the closest "item-like"), and the `local_id` which is unique within the given owner.

This PR is also running a number of consistency checks for the generated `HirId`s:
- Does `NodeId` in the HIR have a corresponding `HirId`?
- Is the `owner` part of each `HirId` consistent with its position in the HIR?
- Do the numerical values of the `local_id` part all lie within a dense range of integers?

cc @rust-lang/compiler

r? @eddyb or @nikomatsakis
@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 27, 2017
bors added a commit that referenced this issue Aug 14, 2017
…rielb1

Use hir::ItemLocalId as keys in TypeckTables.

This PR makes `TypeckTables` use `ItemLocalId` instead of `NodeId` as key. This is needed for incremental compilation -- for stable hashing and for being able to persist and reload these tables. The PR implements the most important part of #40303.

Some notes on the implementation:
* The PR adds the `HirId` to HIR nodes where needed (`Expr`, `Local`, `Block`, `Pat`) which obviates the need to store a `NodeId -> HirId` mapping in crate metadata. Thanks @eddyb for the suggestion! In the future the `HirId` should completely replace the `NodeId` in HIR nodes.
* Before something is read or stored in one of the various `TypeckTables` subtables, the entry's key is validated via the new `TypeckTables::validate_hir_id()` method. This makes sure that we are not mixing information from different items in a single table.

That last part could be made a bit nicer by either (a) new-typing the table-key and making `validate_hir_id()` the only way to convert a `HirId` to the new-typed key, or (b) just encapsulate sub-table access a little better. This PR, however, contents itself with not making things significantly worse.

Also, there's quite a bit of switching around between `NodeId`, `HirId`, and `DefIndex`. These conversions are cheap except for `HirId -> NodeId`, so if the valued reviewer finds such an instance in a performance critical place, please let me know.

Ideally we convert more and more code from `NodeId` to `HirId` in the future so that there are no more `NodeId`s after HIR lowering anywhere. Then the amount of switching should be minimal again.

r? @eddyb, maybe?
@michaelwoerister
Copy link
Member

We implemented HirId a while ago and it works pretty well for what we need it for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants