Allocate HIR on an arena 1/4#66931
Conversation
|
cc @rust-lang/compiler |
|
r? @eddyb |
src/librustc/arena.rs
Outdated
There was a problem hiding this comment.
Maybe call this HIR types and put a newline above it to visually separate it from the rest.
src/librustc/hir/intravisit.rs
Outdated
There was a problem hiding this comment.
I'm curious, why didn't &krate.attrs still work?
There was a problem hiding this comment.
I changed the HirVec<T> to be &'hir [T]. Using &krate.attrs used to make a &[T], and now makes a &&[T].
src/librustc/hir/lowering.rs
Outdated
There was a problem hiding this comment.
The names of these lifetimes are quite misleading. I think they should be named MiscCollector<'misc, 'lowering, 'hir> instead.
src/librustc/hir/lowering/item.rs
Outdated
There was a problem hiding this comment.
This could be named ItemLowerer<'a, 'lowering, 'hir>.
src/librustc/hir/print.rs
Outdated
There was a problem hiding this comment.
This could probably be &hir::Crate<'_>?
src/librustc/hir/lowering/item.rs
Outdated
There was a problem hiding this comment.
This feels noisy and repetitive. Could we bake self.arena.alloc(ty.into_inner()) into lower_ty or if not, for whatever reason, add a lower_ty_arena?
There was a problem hiding this comment.
I'd expect that to happen on the 3rd PR which moves Ty to the arena. I guess @cjgillot moved the fields of each type to the arena too so these show up as temporary changes.
There was a problem hiding this comment.
There doesn't seem to be that many fields that's moved to &'hir Ty though. I'd just keep P<Ty> for this PR to avoid unnecessary changes.
There was a problem hiding this comment.
Indeed, a lot of those disappear in #66942. I plan to do the final cleanup in PR 4/4.
There was a problem hiding this comment.
After discussing with @pnkfelix, I remembered another reason to migrate eagerly the P<Ty> into &'hir Ty. If I do not do that, I get unused lifetime parameter errors in intermediate commits (ex: TraitItemKind). This keeps me from splitting in compiling commits.
src/librustc/arena.rs
Outdated
There was a problem hiding this comment.
This could have a few attribute since they're likely to be rare.
src/librustc/arena.rs
Outdated
There was a problem hiding this comment.
This should probably have a few attribute too.
src/librustc/hir/lowering/item.rs
Outdated
There was a problem hiding this comment.
This seems a bit odd. Shouldn't lower_attrs_extendable still return a Vec?
src/librustc/hir/lowering.rs
Outdated
There was a problem hiding this comment.
This should still return Vec<Attribute> since callers may want to extend it.
There was a problem hiding this comment.
lower_attrs is the function which should return &'hir [Attribute].
There was a problem hiding this comment.
I suggest you add a lower_attrs_arena variant which returns &'hir [Attribute] which can be removed when all the attributes are migrated over.
src/librustc/hir/lowering/item.rs
Outdated
There was a problem hiding this comment.
We don't want to use for<'tcx> here.
|
☔ The latest upstream changes (presumably #66944) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
cjgillot
force-pushed
the
hirene-preamble
branch
from
e2ad73e to
8985208
Compare
|
Rebased with review comments. |
cjgillot
force-pushed
the
hirene-preamble
branch
from
bf0c6fb to
15b76ce
Compare
eddyb
left a comment
eddyb
left a comment
There was a problem hiding this comment.
r=me if no concerns remain from @rust-lang/compiler (also, feel free to ping/PM me on Discord or Zulip in the future, when each PR is ready)
|
☔ The latest upstream changes (presumably #66984) made this pull request unmergeable. Please resolve the merge conflicts. |
cjgillot
force-pushed
the
hirene-preamble
branch
from
15b76ce to
0c4b8ee
Compare
|
My take is that we should land this. Maybe we can nominate for a quick 👍 at triage meeting? Seems like it would be good to get some kind of perf numbers, too. Let's kick that off. @bors try |
|
⌛ Trying commit 0c4b8eee9c9e63f818861bec48e6fd356daf4166 with merge 963a1868bdc29bd60b42fc3c9705b6b186b61927... |
cjgillot
force-pushed
the
hirene-preamble
branch
from
f5ce0e7 to
baa49b2
Compare
|
Rebased. |
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors r=eddyb |
|
📌 Commit baa49b2 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
|
@bors p=100 |
Allocate HIR on an arena 1/4 This PR is the first in a series of 4, aiming at allocating the HIR on an arena, as a memory optimisation. 1. This first PR lays the groundwork and migrates some low-hanging fruits. 2. The second PR will migrate `hir::Expr`, `hir::Pat` and related. 3. The third PR will migrate `hir::Ty` and related. 4. The final PR will be dedicated to eventual cleanups. In order to make the transition as gradual as possible, some lowering routines receive `Box`-allocated data and move it into the arena. This is a bit wasteful, but hopefully temporary. Nonetheless, special care should be taken to avoid double arena allocations. Work mentored by @Zoxc.
|
☀️ Test successful - checks-azure |
Rustup to rust-lang/rust#66931 changelog: none
Allocate HIR on an arena 2/4 -- Expr & Pat This is the second PR in the series started by #66931 This time, commits don't really make sense on their own. They are mostly split by type of compile error. The additional diff is here: cjgillot/rust@hirene-preamble...hirene-expr
Allocate HIR on an arena 3/4 -- Ty This is the third PR in the series started by #66931 and #66936 Once again, commits don't really make sense on their own. They are mostly split by type of compile error. The additional diff is here: cjgillot/rust@hirene-expr...hirene-ty
Allocate HIR on an arena 4/4 This is the fourth and last PR in the series started by #66931, #66936 and #66942. The last commits should compile on their own. The difference with the previous PR is given by cjgillot/rust@hirene-ty...hirene A few more cleanups may be necessary, please tell me. r? @eddyb like the other cc @Zoxc
10 participants
This PR is the first in a series of 4, aiming at allocating the HIR on an arena, as a memory optimisation.
hir::Expr,hir::Patand related.hir::Tyand related.In order to make the transition as gradual as possible, some lowering routines receive
Box-allocated data and move it into the arena. This is a bit wasteful, but hopefully temporary.Nonetheless, special care should be taken to avoid double arena allocations.
Work mentored by @Zoxc.