Refactor builder arena #362
Conversation
marmeladema
force-pushed
the
refactor-builder-arena
branch
from
0787336
to
ab70c2c
Compare
|
There is a small bug in the first commit because the vec of segments is leaked. It is not strictly unsound bit I'll fix it tomorrow anyway. |
There were only two calls to `BuilderArenaImplInner::allocate_segment` and they were both followed by a call to `BuilderArenaImplInner::allocate`.
marmeladema
force-pushed
the
refactor-builder-arena
branch
from
ab70c2c
to
83a1047
Compare
|
@dwrensha this is ready for review :) Unless |
| self.segments.push(BuilderSegment { | ||
| ptr: seg.0, | ||
| capacity: seg.1, | ||
| allocated: 0, | ||
| allocated: minimum_size, |
There was a problem hiding this comment.
This looks wrong to me. allocated is supposed to indicate how many words of the segment are already in use, which should be zero when the segment is first allocated. It seems to me that what you have here would result in a lot of unused memory.
I pushed some new doc comments for BuilderSegment that might make things clearer: 8888443
I suppose one things that's potentially confusing about all this is that the word "allocate" is being used for different things:
- the
message::Allocator, whose job is to provide memory from the system BuilderArena::allocate(), which takes takes memory already-"allocated" memory from (1) and reserves it for usage byprivate::layout.
There was a problem hiding this comment.
Oh, I see. This has to do with commit b6f6931. It happens to work because there are only a two usages of the allocate_segment() method, and they follow a particular pattern.
I'd prefer not to make this change. If we do make it, we should at least add some comments about the new behavior, and maybe also change some naming.
There was a problem hiding this comment.
Maybe we can introduce a dedicated method for this then? I think having a non fallible method helps with readability and optimizations
There was a problem hiding this comment.
@dwrensha would allocate_full_segment be a suitable name for this method? I am not very good with names 😅 Given that this is a private method, maybe it doesn't matter too much? Once decided I will also update the doc comment
| @@ -247,7 +261,12 @@ where | |||
| pub fn into_allocator(self) -> A { | |||
| let mut inner = self.inner.into_inner(); | |||
| inner.deallocate_all(); | |||
| inner.allocator.take().unwrap() | |||
| // See https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/.E2.9C.94.20Extracting.20field.20from.20struct.20that.20implement.20.60Drop.60 | |||
| let (allocator, _segments) = unsafe { | |||
There was a problem hiding this comment.
I'm hesitant to add more unsafe like this. I'm tempted to pull in the changes from #363 without the changes from this PR, to see if that still works well. Getting rid of that RefCell to me is the big win of all this.
There was a problem hiding this comment.
As you wish :-) I will likely be testing all those changes in production in the next couple of weeks.
I do think simplifying the codebase, is worth one unsafe block though.
Each commit should be self explainable.
This makes the code simpler and should help the compiler optimize things a bit better.