arena2: resolve per allocation overhead#61
Conversation
|
I have been contributing to open source since 2 -3 years, when AI coding tools weren't even a thing, if you have any objections with my work you can review it (that's what people in open source do, right?), but I dont think it's the place to show your frustation, since I have been contributing to this repo continuously for the past month, and I guess any one is open to if you want to, the orgs have been announced for a month now, if you are really interested in a project and want to contribute, I don't think anything is stopping you from doing that. Also someone else's work doesn't put down yours |
fyi, the optimizations in my last pr were based on running benchmarks in boa_gc and adding #[inline] to function calls, what appears ai like in this??
I guess every developer is contributing through that noise, since times have changed, but you can't quit on something you love doing just because a lot of people are doing it, again I would say someone else's work doesn't put down yours |
|
this isnt even a Pr you linked, Really in 2025, are we really seeing the same profiles?? anyways I don't need to prove my work to you, if the maintainers have no objection with my work, I don't think your opinion matters, so best of luck I have a lot of work to do, I am not going engage with you anymore |
|
Why was this closed? I was still reviewing the changes |
There were a lot of spam comments on this one, so I thought I would open a new one |
|
Reopened it |
|
Ah, yeah. I was planning on deleting those and cleaning up the PR. Sorry for the delay on that 😓 Definitely spam. A PR is not the place for any of that, especially on a repo like this one 🤣 |
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Feedback is below!
Actually looking through this a bit deeper, I have a sinking feeling that we may not be able to condense the overhead until we change how we track roots.
There is a good chance that this would be fine in the context of our GC, but I don't know if I want to risk it without a way that I feel confident about proving.
| drop_in_place(self.value_mut()) | ||
| } | ||
| } | ||
| // With repr(transparent), the outer struct has the same address as the inner value |
There was a problem hiding this comment.
We should just remove this method. No point preserving two APIs that are returning *mut T
| buf: NonNull<u8>, // Start of a byte buffer | ||
| } | ||
| #[repr(transparent)] | ||
| pub struct ErasedHeapItem(NonNull<u8>); |
| impl<T> core::convert::AsRef<T> for ErasedHeapItem { | ||
| fn as_ref(&self) -> &T { | ||
| // SAFETY: TODO | ||
| // SAFETY: caller ensures this pointer was allocated as T |
| #[derive(Debug, Clone, Copy)] | ||
| #[repr(transparent)] | ||
| pub struct ErasedArenaPointer<'arena>(NonNull<ErasedHeapItem>, PhantomData<&'arena ()>); | ||
| pub struct ErasedArenaPointer<'arena>(NonNull<u8>, PhantomData<&'arena ()>); |
There was a problem hiding this comment.
question: why not use ErasedHeapItem here?
I think I'd prefer to preserve the type for it's explicitness, but I'm open to this if you have a good argument for NonNull<u8>
| pub layout: Layout, | ||
| pub last_allocation: Cell<*mut ErasedHeapItem>, | ||
| /// Number of allocations made in this arena | ||
| alloc_count: Cell<usize>, |
There was a problem hiding this comment.
issue: I'm not sure this is the correct approach.
I think this approach does work, but I believe it would open us up to a pretty large issue where an already dropped allocation could be provided and then the drop count is immediately incorrect.
The allocation header / footer needs to be able to track its liveliness
|
thanks for the review :) |
2 participants
fix #58
converted
ArenaHeapItem<T>into zero costrepr(transparent)wrapper by removing the extra 8 byteTaggedPtrheader so only theGcBoxheader remains. Replaced O(n) linkedlist check for empty arenas with O(1)alloc_count == drop_count checksand movedmark_droppedto the allocator layer. Because objects no longer have hidden headers with raw pointers, we can now safely build features like compacting or generational GCAlso
PoolItemalready worked this way, so no changes made thereI have documented my findings in the notes