Box the biggest ast::ItemKind variants #81405

bugadani · 2021-01-26T07:40:22Z

This PR is a different approach on #81400, aiming to save memory in humongous ASTs.

The three affected item kind enums are:

ast::ItemKind (208 -> 112 bytes)
ast::AssocItemKind (176 -> 72 bytes)
ast::ForeignItemKind (176 -> 72 bytes)

rust-highfive · 2021-01-26T07:40:26Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2021-01-26T14:33:32Z

@bors try @rust-timer queue

rust-timer · 2021-01-26T14:33:34Z

Awaiting bors try build completion.

bors · 2021-01-26T14:33:46Z

⌛ Trying commit 7b379a25a2ac7d8dd3508f73e5cdec3e8a34452d with merge 7fc85fb97c995b5a91e77a8c4addcd865da24d11...

bors · 2021-01-26T15:24:01Z

☀️ Try build successful - checks-actions
Build commit: 7fc85fb97c995b5a91e77a8c4addcd865da24d11 (7fc85fb97c995b5a91e77a8c4addcd865da24d11)

rust-timer · 2021-01-26T15:24:03Z

Queued 7fc85fb97c995b5a91e77a8c4addcd865da24d11 with parent 1483e67, future comparison URL.

@rustbot label: +S-waiting-on-perf

rust-timer · 2021-01-26T19:25:06Z

Finished benchmarking try commit (7fc85fb97c995b5a91e77a8c4addcd865da24d11): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

bugadani · 2021-01-26T19:32:34Z

Wait, what did I do to make typeck faster? I thought that works on the HIR and not the AST o.O Anyway, small runtime improvement and a disappointing lack of improvement in max-rss.

In comparison, a bit less perf gain than #81400, but opposed to that this is at least not completely red on memory.

Mark-Simulacrum · 2021-01-27T13:59:07Z

@bors try @rust-timer queue

rust-timer · 2021-01-27T13:59:08Z

Awaiting bors try build completion.

bors · 2021-01-27T13:59:19Z

⌛ Trying commit ff464fe5d23cee78979bf8669c8c83ef7ffcb2b7 with merge bf1b6dbd7f67fc51be6c4ba42a6d0d25a6fa9a3b...

bors · 2021-01-27T14:59:50Z

☀️ Try build successful - checks-actions
Build commit: bf1b6dbd7f67fc51be6c4ba42a6d0d25a6fa9a3b (bf1b6dbd7f67fc51be6c4ba42a6d0d25a6fa9a3b)

rust-timer · 2021-01-27T14:59:51Z

Queued bf1b6dbd7f67fc51be6c4ba42a6d0d25a6fa9a3b with parent 613ef74, future comparison URL.

@rustbot label: +S-waiting-on-perf

rust-timer · 2021-01-27T16:38:47Z

Finished benchmarking try commit (bf1b6dbd7f67fc51be6c4ba42a6d0d25a6fa9a3b): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

bugadani · 2021-01-27T16:45:05Z

Perf is largely the same as before, max-rss maybe a bit better (uh, less terrible?) if you squint real hard, but that can always be noise.

bugadani · 2021-01-29T07:32:41Z

I undid most of the clippy formatting changes (thanks, RA), and I declare this ready for review.

bors · 2021-01-31T03:11:19Z

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

cjgillot · 2021-01-31T14:33:06Z

Can you add a static_assert_size! on Item, TraitItem, ImplItem and ForeignItem? This will allow to avoid detect regressions.
Do you have the sizes of those structs before this PR?

cjgillot · 2021-01-31T14:38:50Z

compiler/rustc_ast/src/ast.rs

+}
+
+#[derive(Clone, Encodable, Decodable, Debug)]
+pub struct FnKind(pub Defaultness, pub FnSig, pub Generics, pub Option<P<Block>>);


Should these new structs have named fields?

Honestly, I'm undecided. These were fine without before, and adding names would just bloat up a lot of match patterns. If we were to start replacing them here, we could go through the whole codebase and replace like two million tuple structs as well.

compiler/rustc_ast/src/ast.rs

bugadani · 2021-01-31T19:09:04Z

Can you add a static_assert_size! on Item, TraitItem, ImplItem and ForeignItem?

Sure, but I'm not sure if the structs have the same size on 32bit platforms - wouldn't that be an issue, either now or later?

cjgillot · 2021-01-31T21:50:13Z

Can you add a static_assert_size! on Item, TraitItem, ImplItem and ForeignItem?

Sure, but I'm not sure if the structs have the same size on 32bit platforms - wouldn't that be an issue, either now or later?

They wouldn't be the same. In rustc_hir, those asserts are only enabled on x86_64, and mostly serve as an indication to the reader.

Precision:

#[cfg(target_arch = "x86_64")]
rustc_data_structures::static_assert_size!(hir::Expr<'static>, 72);

bugadani · 2021-02-01T08:38:15Z

@cjgillot I've added asserts (to the enums, because it's their sizes we want to keep low to avoid wasting memory for the "smaller" variants), and I've updated the PR with the size changes.

There are two more possibilities for the near future, based on this PR:

While gathering the sizes, I've noticed that some of the smaller variants in ItemKind are behind a pointer. I wonder if they are small enough to be inlined into the enum. I'll try that in a followup PR.

Also, #81400 showed that perf-wise there is not much difference between boxing a few variants vs boxing the whole enum. Since this PR aims at saving memory, I'll check if it's worth taking the current approach to the extremes and making ItemKind even smaller. This kind-of conflicts with the previous point, though.

cjgillot · 2021-02-02T17:22:17Z

@bors r+ rollup=never

bors · 2021-02-02T17:22:19Z

📌 Commit 003fba3 has been approved by cjgillot

bors · 2021-02-02T17:34:14Z

⌛ Testing commit 003fba3 with merge 3182375...

bors · 2021-02-02T20:27:03Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 3182375 to master...

Box the biggest ast::ItemKind variants This PR is a different approach on rust-lang#81400, aiming to save memory in humongous ASTs. The three affected item kind enums are: - `ast::ItemKind` (208 -> 112 bytes) - `ast::AssocItemKind` (176 -> 72 bytes) - `ast::ForeignItemKind` (176 -> 72 bytes)

rust-highfive assigned Mark-Simulacrum Jan 26, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2021

bugadani changed the title ~~Box the biggest ast::ItemKind variants~~ [Experiment] Box the biggest ast::ItemKind variants Jan 26, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 27, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 27, 2021

bugadani force-pushed the ast branch from ff464fe to 7e24f99 Compare January 29, 2021 07:31

bugadani changed the title ~~[Experiment] Box the biggest ast::ItemKind variants~~ Box the biggest ast::ItemKind variants Jan 29, 2021

davidtwco mentioned this pull request Jan 30, 2021

Box ast::ItemKind #81400

Closed

cjgillot reviewed Jan 31, 2021

View reviewed changes

compiler/rustc_ast/src/ast.rs Show resolved Hide resolved

bugadani added 2 commits February 1, 2021 09:23

Box the biggest ast::ItemKind variants

b87e1ec

Assert the size of the refactored enums

003fba3

bugadani force-pushed the ast branch from 7e24f99 to 003fba3 Compare February 1, 2021 08:23

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2021

bors merged commit 3182375 into rust-lang:master Feb 2, 2021

rustbot added this to the 1.51.0 milestone Feb 2, 2021

bors mentioned this pull request Feb 2, 2021

resolve: Cleanup visibility resolution for enum variants and trait items #80762

Merged

bugadani deleted the ast branch February 2, 2021 20:44

bors mentioned this pull request Feb 2, 2021

Only store a LocalDefId in some HIR nodes #81611

Merged

bugadani mentioned this pull request Feb 2, 2021

Box all fields of ast::*ItemKind #81683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Box the biggest ast::ItemKind variants #81405

Box the biggest ast::ItemKind variants #81405

bugadani commented Jan 26, 2021 •

edited

Loading

rust-highfive commented Jan 26, 2021

Mark-Simulacrum commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bors commented Jan 26, 2021

bors commented Jan 26, 2021

rust-timer commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bugadani commented Jan 26, 2021 •

edited

Loading

Mark-Simulacrum commented Jan 27, 2021

rust-timer commented Jan 27, 2021

bors commented Jan 27, 2021

bors commented Jan 27, 2021

rust-timer commented Jan 27, 2021

rust-timer commented Jan 27, 2021

bugadani commented Jan 27, 2021

bugadani commented Jan 29, 2021

bors commented Jan 31, 2021

cjgillot commented Jan 31, 2021

cjgillot Jan 31, 2021

bugadani Jan 31, 2021

bugadani commented Jan 31, 2021

cjgillot commented Jan 31, 2021 •

edited

Loading

bugadani commented Feb 1, 2021

cjgillot commented Feb 2, 2021

bors commented Feb 2, 2021

bors commented Feb 2, 2021

bors commented Feb 2, 2021

Box the biggest ast::ItemKind variants #81405

Box the biggest ast::ItemKind variants #81405

Conversation

bugadani commented Jan 26, 2021 • edited Loading

rust-highfive commented Jan 26, 2021

Mark-Simulacrum commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bors commented Jan 26, 2021

bors commented Jan 26, 2021

rust-timer commented Jan 26, 2021

rust-timer commented Jan 26, 2021

bugadani commented Jan 26, 2021 • edited Loading

Mark-Simulacrum commented Jan 27, 2021

rust-timer commented Jan 27, 2021

bors commented Jan 27, 2021

bors commented Jan 27, 2021

rust-timer commented Jan 27, 2021

rust-timer commented Jan 27, 2021

bugadani commented Jan 27, 2021

bugadani commented Jan 29, 2021

bors commented Jan 31, 2021

cjgillot commented Jan 31, 2021

cjgillot Jan 31, 2021

Choose a reason for hiding this comment

bugadani Jan 31, 2021

Choose a reason for hiding this comment

bugadani commented Jan 31, 2021

cjgillot commented Jan 31, 2021 • edited Loading

bugadani commented Feb 1, 2021

cjgillot commented Feb 2, 2021

bors commented Feb 2, 2021

bors commented Feb 2, 2021

bors commented Feb 2, 2021

bugadani commented Jan 26, 2021 •

edited

Loading

bugadani commented Jan 26, 2021 •

edited

Loading

cjgillot commented Jan 31, 2021 •

edited

Loading