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

Allocate HIR on an arena 1/4 #66931

Merged
merged 12 commits into from Dec 22, 2019
Merged

Allocate HIR on an arena 1/4 #66931

merged 12 commits into from Dec 22, 2019

Conversation

@cjgillot
Copy link
Contributor

cjgillot commented Dec 1, 2019

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.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Dec 1, 2019

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 1, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned matthewjasper Dec 1, 2019
@@ -123,6 +122,10 @@ macro_rules! arena_types {
[few] crate_variances: rustc::ty::CrateVariancesMap<'tcx>,
[few] inferred_outlives_crate: rustc::ty::CratePredicatesMap<'tcx>,
[] upvars: rustc_data_structures::fx::FxIndexMap<rustc::hir::HirId, rustc::hir::Upvar>,
// HIR nodes arenas

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

Maybe call this HIR types and put a newline above it to visually separate it from the rest.

visitor.visit_mod(&krate.module, krate.span, CRATE_HIR_ID);
walk_list!(visitor, visit_attribute, &krate.attrs);
walk_list!(visitor, visit_macro_def, &krate.exported_macros);
walk_list!(visitor, visit_attribute, krate.attrs);

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

I'm curious, why didn't &krate.attrs still work?

This comment has been minimized.

Copy link
@cjgillot

cjgillot Dec 1, 2019

Author Contributor

I changed the HirVec<T> to be &'hir [T]. Using &krate.attrs used to make a &[T], and now makes a &&[T].

/// Full-crate AST visitor that inserts into a fresh
/// `LoweringContext` any information that may be
/// needed from arbitrary locations in the crate,
/// e.g., the number of lifetime generic parameters
/// declared for every type and trait definition.
struct MiscCollector<'tcx, 'interner> {
lctx: &'tcx mut LoweringContext<'interner>,
struct MiscCollector<'tcx, 'interner, 'hir> {

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

The names of these lifetimes are quite misleading. I think they should be named MiscCollector<'misc, 'lowering, 'hir> instead.

@@ -25,11 +25,11 @@ use syntax_pos::Span;

use rustc_error_codes::*;

pub(super) struct ItemLowerer<'tcx, 'interner> {
pub(super) lctx: &'tcx mut LoweringContext<'interner>,
pub(super) struct ItemLowerer<'tcx, 'interner, 'hir> {

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This could be named ItemLowerer<'a, 'lowering, 'hir>.

@@ -107,7 +107,7 @@ pub const INDENT_UNIT: usize = 4;
/// it can scan the input text for comments to copy forward.
pub fn print_crate<'a>(cm: &'a SourceMap,
sess: &ParseSess,
krate: &hir::Crate,
krate: &hir::Crate<'a>,

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This could probably be &hir::Crate<'_>?

ImplTraitContext::Disallowed(ImplTraitPosition::Binding)
}
),
self.arena.alloc(ty.into_inner()),

This comment has been minimized.

Copy link
@Centril

Centril Dec 1, 2019

Member

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?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

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.

This comment has been minimized.

Copy link
@cjgillot

cjgillot Dec 1, 2019

Author Contributor

Indeed, a lot of those disappear in #66942. I plan to do the final cleanup in PR 4/4.

This comment has been minimized.

Copy link
@cjgillot

cjgillot Dec 11, 2019

Author Contributor

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.

@@ -125,7 +125,12 @@ macro_rules! arena_types {
// HIR nodes arenas
[few] hir_forest: rustc::hir::map::Forest<$tcx>,
[] attribute: syntax::ast::Attribute,
[] global_asm: rustc::hir::GlobalAsm,

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This could have a few attribute since they're likely to be rare.

// HIR nodes arenas
[few] hir_forest: rustc::hir::map::Forest<$tcx>,
[] attribute: syntax::ast::Attribute,
[] macro_def: rustc::hir::MacroDef,

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This should probably have a few attribute too.

let def_id = self.resolver.definitions().local_def_id(i.id);
hir::ForeignItem {
hir_id: self.lower_node_id(i.id),
ident: i.ident,
attrs: self.lower_attrs(&i.attrs),
attrs: self.lower_attrs_extendable(&i.attrs),

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This seems a bit odd. Shouldn't lower_attrs_extendable still return a Vec?

.iter()
.map(|a| self.lower_attr(a))
.collect()
fn lower_attrs_extendable(&mut self, attrs: &[Attribute]) -> &'hir [Attribute] {

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

This should still return Vec<Attribute> since callers may want to extend it.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

lower_attrs is the function which should return &'hir [Attribute].

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

I suggest you add a lower_attrs_arena variant which returns &'hir [Attribute] which can be removed when all the attributes are migrated over.

@@ -1042,7 +1042,7 @@ impl LoweringContext<'_, 'hir> {

fn lower_body(
&mut self,
f: impl FnOnce(&mut LoweringContext<'_, '_>) -> (HirVec<hir::Param>, hir::Expr),
f: impl for<'tcx> FnOnce(&mut LoweringContext<'_, 'tcx>) -> (&'tcx [hir::Param], hir::Expr),

This comment has been minimized.

Copy link
@Zoxc

Zoxc Dec 1, 2019

Contributor

We don't want to use for<'tcx> here.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 2, 2019

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

@cjgillot cjgillot force-pushed the cjgillot:hirene-preamble branch from e2ad73e to 8985208 Dec 2, 2019
@cjgillot

This comment has been minimized.

Copy link
Contributor Author

cjgillot commented Dec 3, 2019

Rebased with review comments.

@cjgillot cjgillot force-pushed the cjgillot:hirene-preamble branch from bf0c6fb to 15b76ce Dec 3, 2019
@eddyb
eddyb approved these changes Dec 5, 2019
Copy link
Member

eddyb left a 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)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

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

@cjgillot cjgillot force-pushed the cjgillot:hirene-preamble branch from 15b76ce to 0c4b8ee Dec 9, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 9, 2019

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

⌛️ Trying commit 0c4b8ee with merge 963a186...

@cjgillot cjgillot force-pushed the cjgillot:hirene-preamble branch from f5ce0e7 to baa49b2 Dec 21, 2019
@cjgillot

This comment has been minimized.

Copy link
Contributor Author

cjgillot commented Dec 21, 2019

Rebased.

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 22, 2019

@bors retry

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 22, 2019

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit baa49b2 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Dec 22, 2019

@bors p=100

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

⌛️ Testing commit baa49b2 with merge 26286c7...

bors added a commit that referenced this pull request Dec 22, 2019
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.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 26286c7 to master...

@bors bors added the merged-by-bors label Dec 22, 2019
@bors bors merged commit baa49b2 into rust-lang:master Dec 22, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191221.66 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Dec 22, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Dec 22, 2019
Rustup to rust-lang/rust#66931

changelog: none
@cjgillot cjgillot deleted the cjgillot:hirene-preamble branch Dec 22, 2019
bors added a commit that referenced this pull request Dec 27, 2019
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
bors added a commit that referenced this pull request Dec 27, 2019
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
bors added a commit that referenced this pull request Dec 29, 2019
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
bors added a commit that referenced this pull request Dec 29, 2019
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
bors added a commit that referenced this pull request Dec 31, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.