Skip to content

Use a simple arena for annotatable items - #162006

Closed
Kobzol wants to merge 1 commit into
rust-lang:mainfrom
Kobzol:annotable-arena
Closed

Use a simple arena for annotatable items#162006
Kobzol wants to merge 1 commit into
rust-lang:mainfrom
Kobzol:annotable-arena

Conversation

@Kobzol

@Kobzol Kobzol commented Aug 30, 2026

Copy link
Copy Markdown
Member

When profiling bors with dhat, this was one of the places that were kinda hot for allocations. So I tried to use an amortized Vec for the annotatable items. I didn't store it in MacroExpander, because those are created repeatedly, but rather in ExtCtxt, so that it survives over multiple expansion calls, and the memory can be better reused.

Since we already pass &mut ExtCtxt to the expansion functions, I couldn't easily pass &mut Vec<Annotatable> to all the expansion functions too, because that would be a double &mut reference. So as an experiment for a benchmark, I just switched all the functions to directly push to the arena in ExtCtxt. This is not super pretty and only works since the ExtCtxt is currently not used from multiple threads. I wanted to refactor it, but I figured that it's maybe not so egregious? So first wanted to get a second opinion before I do that.

If we wanted to do this via &mut Vec<Annotatable>, I could store RefCell<Vec<Annotatable>> in ExtCtxt, and borrow_mut() it just before calling the expansion. But for that to work, I'd probably have to replace &mut ExtCtxt in the expansion calls (which might? be possible), or store Rc<RefCell<Vec<Annotatable>>> instead.

r? nnethercote

@rustbot

rustbot commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs

cc @ZuseZ4

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 30, 2026
@Kobzol

Kobzol commented Aug 30, 2026

Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 30, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 30, 2026
Use a simple arena for annotatable items
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Diff in /checkout/compiler/rustc_builtin_macros/src/deriving/generic/mod.rs:546:
                         })
                         .cloned(),
                 );
-                cx.annotatable_arena
-                    .push(transform(Annotatable::Item(Box::new(ast::Item { attrs, ..(*newitem).clone() }))));
+                cx.annotatable_arena.push(transform(Annotatable::Item(Box::new(ast::Item {
+                    attrs,
+                    ..(*newitem).clone()
+                }))));
             }
             _ => unreachable!(),
         }
fmt: checked 7206 files
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`

@rust-bors

rust-bors Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: fc9f757 (fc9f7579dad04fdfc8d4534004d9a05d9f9f7555)
Base parent: 3cabe36 (3cabe36ceb022e2f56d4d330b1e2886f31117f18)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (fc9f757): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.5%, secondary -3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.5% [2.9%, 4.1%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-4.6%, -2.1%] 3
All ❌✅ (primary) 3.5% [2.9%, 4.1%] 2

Cycles

Results (secondary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.2% [2.0%, 2.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 3
All ❌✅ (primary) - - 0

Bootstrap: 475.389s -> 502.254s (5.65%)
Artifact size: 402.85 MiB -> 402.88 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 30, 2026
@Kobzol

Kobzol commented Aug 30, 2026

Copy link
Copy Markdown
Member Author

Damn, I think I benchmarked this with an unrelated change before, and the win came from that change alone. Good thing I didn't do the painful refactoring :D Nevermind, closing.

@Kobzol Kobzol closed this Aug 30, 2026
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2026
@Kobzol
Kobzol deleted the annotable-arena branch August 30, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants