-
Notifications
You must be signed in to change notification settings - Fork 4.6k
perf: cache thread_id::current() in a #[thread_local] slot #30971
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
base: main
Are you sure you want to change the base?
Changes from all commits
ee6c3be
d4252e9
766b552
a07bd69
1dac3fb
e035622
b14f5ee
3095c91
beccfc2
44fc2ae
e076274
f2a0af6
3a5d33e
4aa39de
f91c936
74017e2
c05b341
e856dd8
696e136
e2f6a24
2f17a5c
e9eec9f
624bb56
e21d0b8
1d8e785
1d1f2ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1137,7 +1137,7 @@ pub struct Part { | |
| } | ||
|
|
||
| pub type PartImportRecordIndices = Vec<u32, bun_alloc::AstAlloc>; | ||
| pub type PartList = Vec<Part>; | ||
| pub type PartList<'a> = bun_alloc::ArenaVec<'a, Part>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🤖 Prompt for AI Agents |
||
|
|
||
| #[derive(Copy, Clone, PartialEq, Eq, Debug)] | ||
| pub enum PartTag { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,14 +97,32 @@ static BUMP_CUR: Cell<*mut u8> = Cell::new(core::ptr::null_mut()); | |
| /// One-past-the-end of the active bump chunk. | ||
| #[thread_local] | ||
| static BUMP_END: Cell<*mut u8> = Cell::new(core::ptr::null_mut()); | ||
| /// The `mi_heap_t*` that owns the active bump chunk (set in [`bump_refill`]). | ||
| /// Lets [`set_thread_heap`] keep the cursor when re-entering the same heap so | ||
| /// the bundler's per-task `push()`/`pop()` doesn't abandon a 16 KB chunk every | ||
| /// task — that was ~70 K tasks × 16 KB ≈ 1.1 GB of mostly-empty chunks held in | ||
| /// the never-reset worker `ast_memory_store` arenas. | ||
| #[thread_local] | ||
| static BUMP_HEAP: Cell<*mut mimalloc::Heap> = Cell::new(core::ptr::null_mut()); | ||
|
|
||
| /// Drop the bump cursor (the chunk itself is owned by `AST_HEAP` and reclaimed | ||
| /// by `mi_heap_destroy`). Called on every [`set_thread_heap`] so a cursor never | ||
| /// outlives the heap that backs its chunk. | ||
| /// Drop the bump cursor (the chunk itself is owned by `BUMP_HEAP` and reclaimed | ||
| /// by `mi_heap_destroy`). | ||
| #[inline] | ||
| fn bump_reset() { | ||
| BUMP_CUR.set(core::ptr::null_mut()); | ||
| BUMP_END.set(core::ptr::null_mut()); | ||
| BUMP_HEAP.set(core::ptr::null_mut()); | ||
| } | ||
|
|
||
| /// Invalidate the bump cursor if it is backed by `heap`. Called from | ||
| /// `MimallocArena::reset`/`Drop` immediately before `mi_heap_destroy(heap)` so | ||
| /// a recycled `mi_heap_t*` slot can't ABA-match `BUMP_HEAP` and serve a stale | ||
| /// (freed) chunk under [`set_thread_heap`]. | ||
| #[inline] | ||
| pub(crate) fn bump_invalidate_heap(heap: *mut mimalloc::Heap) { | ||
| if BUMP_HEAP.get() == heap { | ||
| bump_reset(); | ||
| } | ||
| } | ||
|
|
||
| /// Carve `size` bytes at `align` (a power of two `<= MI_MAX_ALIGN_SIZE`) from | ||
|
|
@@ -149,6 +167,7 @@ fn bump_refill(heap: *mut mimalloc::Heap, size: usize) -> Option<*mut u8> { | |
| // both `add`s are in bounds. | ||
| BUMP_CUR.set(unsafe { chunk.add(size) }); | ||
| BUMP_END.set(unsafe { chunk.add(BUMP_CHUNK) }); | ||
| BUMP_HEAP.set(heap); | ||
| Some(chunk) | ||
| } | ||
|
|
||
|
|
@@ -172,7 +191,16 @@ fn bump_refill(heap: *mut mimalloc::Heap, size: usize) -> Option<*mut u8> { | |
| #[inline] | ||
| pub fn set_thread_heap(heap: *mut mimalloc::Heap) { | ||
| AST_HEAP.set(heap); | ||
| bump_reset(); | ||
| // Keep the cursor when re-entering the heap that owns it (the bundler's | ||
| // per-task `push()`/`pop()` always passes the same per-worker arena). Only | ||
| // discard when switching to a *different* non-null heap — the chunk then | ||
| // belongs to the wrong owner. Clearing to null is a no-op for the cursor | ||
| // (no allocs happen while `AST_HEAP` is null), so it survives the | ||
| // `pop()→push()` round-trip. Heap destruction must call [`bump_reset`] | ||
| // explicitly to defend against address reuse. | ||
| if !heap.is_null() && heap != BUMP_HEAP.get() { | ||
| bump_reset(); | ||
| } | ||
|
Comment on lines
192
to
+203
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't preserve the bump cursor across
Safe fallback pub fn set_thread_heap(heap: *mut mimalloc::Heap) {
AST_HEAP.set(heap);
- if !heap.is_null() && heap != BUMP_HEAP.get() {
+ if heap.is_null() || heap != BUMP_HEAP.get() {
bump_reset();
}
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /// Current thread's AST heap, or null if no `ASTMemoryAllocator` scope is | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The newly-added comment says "use
Ast::empty_in(arena)(orAst::empty()with the process-static arena for placeholder values)", but this same PR deletesAst::empty()— onlyempty_in(arena)exists now. The parenthetical should be dropped.Extended reasoning...
What the issue is
The PORT NOTE comment added above
impl<'a> Ast<'a>at src/ast/ast_result.rs:104-106 reads:However, this PR also deletes the
Ast::empty()function. The diff shows the old definition being removed:After this PR, the only constructor is
pub fn empty_in(arena: &'a bun_alloc::MimallocArena) -> Self. There is noAst::empty()and no "process-static arena" overload.Why nothing prevents it
This is purely a documentation comment, so the compiler doesn't check it. The comment was likely written at an intermediate stage of the refactor when both
empty()andempty_in()coexisted, and the parenthetical wasn't updated whenempty()was finally removed.Step-by-step proof
Ast::empty_in(arena)(orAst::empty()with the process-static arena for placeholder values)".fn empty(— onlyempty_inmatches;empty()does not exist.impl Astblock explicitly removespub fn empty() -> Ast { Ast::default() }.Impact
No runtime impact — it's a code comment. The risk is that a future reader follows the hint, tries to call
Ast::empty(), and gets a compile error / wastes time looking for it. Since this comment is the canonical "how do I get a defaultAstnow?" guidance, it should be accurate.Fix
Drop the parenthetical so the sentence ends at
Ast::empty_in(arena):