recycle empty arenas in arena2 and mempool3 allocator#44
recycle empty arenas in arena2 and mempool3 allocator#44nekevss merged 3 commits intoboa-dev:mainfrom
Conversation
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Merge conflicts need to be resolved. Here's a couple things I noticed too while looking through the PR.
| unsafe { self.inner_ptr.to_typed_arena_pointer::<GcBox<T>>() } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
question: why are these being marked as dead code?
It would be nice to leave a comment, e.g.:
#[allow(dead_code)] // TODO: determine what to do with these or something
oscars/src/alloc/arena2/mod.rs
Outdated
| heap_threshold: DEFAULT_HEAP_THRESHOLD, | ||
| arena_size: DEFAULT_ARENA_SIZE, | ||
| arenas: LinkedList::default(), | ||
| recycled_arenas: rust_alloc::vec::Vec::new(), |
There was a problem hiding this comment.
thought: if we are setting a max value. Then we shouldn't use a vec and opt for an array with the upper bound set by a const.
Resolved |
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Had a few questions that popped up while looking back through this.
Overall though, this is looking pretty good.
|
|
||
| /// Reset arena to its initial empty state, reusing the existing OS buffer. | ||
| /// Must only be called when `run_drop_check()` is true (all items dropped). | ||
| pub fn reset(&self) { |
There was a problem hiding this comment.
Thought: we should probably zero out the memory here to as a proper reset to be "good citizens" so to speak.
I'm not convinced that blocks this PR though.
There was a problem hiding this comment.
Added write_bytes(0) over the full layout size to zero the buffer. This prevents stale object graphs from being observable through any future partial walk of a recycled arena
oscars/src/alloc/arena2/mod.rs
Outdated
| for dead_arenas in self.arenas.extract_if(|a| a.run_drop_check()) { | ||
| drop(dead_arenas) | ||
| let dead_arenas: rust_alloc::vec::Vec<Arena> = | ||
| self.arenas.extract_if(|a| a.run_drop_check()).collect(); |
There was a problem hiding this comment.
question: do we need to collect here?
This returns an iterator correct? So shouldn't we be able to iterate on it directly?
There was a problem hiding this comment.
yes, collect() was unnecessary overhead here, removed it
|
|
||
| // Allocate again, must reuse the recycled arena without growing OS footprint. | ||
| // heap_size returns to the same value as when a live arena was present. | ||
| for i in 16..32 { |
There was a problem hiding this comment.
Does this show us that the arena is recycled? How do we know it's recycled the same memory?
There was a problem hiding this comment.
heap_size() equality proves that the OS footprint didn't grow, but you're right it doesnt prove that the recycled slot was actually consumed. Sorry missed that 😅
I have added two recycled_count assertions to the test to verify that,
recycled_count == 1 after drop_dead_arenas() - confirms the arena was saved for reuse instead of being freed.
recycled_count == 0 after the second alloc loop - confirms initialize_new_arena reused that saved arena instead of asking the OS for new memory
Together these two checks confirm that the recycling process worked correctly.
oscars/src/alloc/mempool3/mod.rs
Outdated
| let empties: Vec<SlotPool> = self | ||
| .slot_pools | ||
| .extract_if(.., |p| p.run_drop_check()) | ||
| .collect(); |
There was a problem hiding this comment.
Re: why can't we use the iterator directly in this instance?
2 participants
arena2andmempool3by holding onto empty memory pages to reuse later, rather than giving them back to the OS immediatelyself.allocator.borrow_mut().drop_empty_pools();call fromcollect()