Store items out-of-line in the HIR #29903
Conversation
| // test base hard to maintain. So instead we sort by node-id | ||
| // so as to get reproducible results. | ||
| let mut pairs: Vec<_> = self.items.iter().collect(); | ||
| pairs.sort_by(|&(id1, _), &(id2, _)| id1.cmp(id2)); |
There was a problem hiding this comment.
Couldn't this be pre-computed? Or use BTreeMap instead.
Or compact Item IDs instead of Node IDs.
There was a problem hiding this comment.
@eddyb yeah, I was debating about this. A BtreeMap would certainly be easy enough to do for now. Compact item-ids seems like the best solution all around perhaps, but I feel like I'd prefer to do that sort of change as a separate patch.
|
r+ with the comment (this is surprisingly straightforward, nice). |
|
☔ The latest upstream changes (presumably #29882) made this pull request unmergeable. Please resolve the merge conflicts. |
| trans_mod(&ccx.rotate(), m); | ||
| hir::ItemMod(_) => { | ||
| // modules have no equivalent at runtime, they just affect | ||
| // the mangled names of things contained within |
There was a problem hiding this comment.
Doesn't this make -C codegen-units ineffective?
There was a problem hiding this comment.
@sanxiyn oh, good catch! I totally overlooked that, actually!
nikomatsakis
force-pushed
the
incr-comp-ool-items
branch
2 times, most recently
from
63bc092
to
ca270ec
Compare
|
I had @michaelwoerister review last commit modifying trans. |
|
@bors r=nrc,mw Hopefully this will pass. I've not been able to run make check locally to completion due to various local problems. |
|
📌 Commit a37317f has been approved by |
|
⌛ Testing commit a37317f with merge a3ee8bb... |
|
💔 Test failed - auto-mac-64-opt |
|
@bors r=nrc,mw |
|
📌 Commit fb28d48 has been approved by |
This PR moves items into a separate map stored in the krate, rather than storing them inline in the HIR. The HIR visitor is also modified to skip visiting nested items by default. The goal here is to ensure that if you get access to the HIR for one item, you don't automatically get access to a bunch of other items, for better dependency tracking. r? @nrc cc @eddyb
|
💔 Test failed - auto-mac-64-nopt-t |
|
@bors r=nrc,mw |
|
📌 Commit a7fd7b0 has been approved by |
|
@bors r- found some local problems |
|
@bors r=nrc,mw |
|
📌 Commit 66045fe has been approved by |
|
☔ The latest upstream changes (presumably #29083) made this pull request unmergeable. Please resolve the merge conflicts. |
walking the patterns in a type fn decl, but those patterns are ignored by this visitor anyway.
instead of `Cell` (but stop short of actualling switching to `&mut`)
rather being stored inline. Refactor (and rename) the visitor so that (by default) it only visits the interior content of an item not nested items. This is a [breaking-change] for anyone who uses the HIR visitor. Besides changing `visit::` to `intravisit::`, you need to refactor your visitor in one of two ways, depending on what it requires: 1. If you just want to visit all items (most common), you should call `krate.visit_all_items(&mut visitor)`. 2. If you need to visit nested items in the middle of the parent items, you should override `visit_nested_item` with something like: `self.visit_item(self.tcx.map.expect_item(item.id))`, presuming you have access to a tcx (or at least a HIR map).
visit nested items). This is what all clients wanted anyhow.
straightforward uses of `visit_all_items`. In some cases I had to remove empty `visit_item` calls that were just to suppress visiting nested items.
the main fn appeared at the top level, if now consults the `DefPath` to get this information
to do "in-situ" visits.
noteworthy because trans got mildly simpler, since it doesn't have to ensure that we walk the contents of all things just to find all the hidden items.
that we no longer use FnvHashMap
encounter each module. This is somewhat different than how it used to
work; it should ensure a more equitable distribution of work than
before. The reason is that, before, when we rotated, we would rotate
before we had seen the full contents of the current module. So e.g. if
we have `mod a { mod b { .. } .. }`, then we rotate when we encounter
`b`, but we haven't processed the remainder of `a` yet. Unclear if this
makes any difference in practice, but it seemed suboptimal. Also, this
structure (with an outer walk over modules) is closer to what we will
want for an incremental setting.
listed (an improvement, I think).
under some configurations this still causes a stack overflow and hence a crash
nikomatsakis
force-pushed
the
incr-comp-ool-items
branch
from
66045fe
to
7926fa1
Compare
|
@bors r=mw,nrc |
|
📌 Commit 7926fa1 has been approved by |
This PR moves items into a separate map stored in the krate, rather than storing them inline in the HIR. The HIR visitor is also modified to skip visiting nested items by default. The goal here is to ensure that if you get access to the HIR for one item, you don't automatically get access to a bunch of other items, for better dependency tracking. r? @nrc cc @eddyb
|
⌛ Testing commit 7926fa1 with merge 9303055... |
This PR moves items into a separate map stored in the krate, rather than storing them inline in the HIR. The HIR visitor is also modified to skip visiting nested items by default. The goal here is to ensure that if you get access to the HIR for one item, you don't automatically get access to a bunch of other items, for better dependency tracking.
r? @nrc
cc @eddyb