fix(arrow/memory): Align allocations always #289
Conversation
|
I still need to add unit tests. |
|
Hmm, something about the memory accounting is no longer right... |
There was a problem hiding this comment.
Any particular reason to do it this way as opposed to just making the Mallocator always aligned (like the Go allocator does) and not having to add that logic to the buffer?
At a worst case you could essentially have the mallocator keep a map[uintptr]uintptr to map the slice data pointer to the true allocation pointer or something.
| // AlignedAllocator is an Allocator that returns buffers aligned to 64-byte boundaries. | ||
| type AlignedAllocator interface { | ||
| Allocator | ||
| // AllocateAligned returns a buffer of the requested size aligned to 64-byte boundaries. buf is the aligned buffer, and alloc is the actual allocation that should be passed to Free. If alloc is nil, then it is the same as buf. |
There was a problem hiding this comment.
can we make this a multi-line comment just to make things cleaner?
| ReallocateAligned(size int, buf []byte, alloc []byte) (newBuf []byte, newAlloc []byte) | ||
| } | ||
|
|
||
| // MakeAlignedAllocator makes an AlignedAllocator for the given Allocator. If the Allocator already implements AlignedAllocator, it is returned as-is. There is usually no need to call this directly as Buffer will call this for you. |
There was a problem hiding this comment.
same here, can we make this a multi-line comment instead?
| @@ -25,3 +25,58 @@ type Allocator interface { | |||
| Reallocate(size int, b []byte) []byte | |||
| Free(b []byte) | |||
| } | |||
|
|
|||
| // AlignedAllocator is an Allocator that returns buffers aligned to 64-byte boundaries. | |||
There was a problem hiding this comment.
any particular reason to pick 64-byte boundary as opposed to letting it be configurable?
| type alignedAllocator struct { | ||
| mem Allocator | ||
| } | ||
|
|
||
| func (a *alignedAllocator) Allocate(size int) []byte { | ||
| return a.mem.Allocate(size) | ||
| } | ||
|
|
||
| func (a *alignedAllocator) Reallocate(size int, b []byte) []byte { | ||
| return a.mem.Reallocate(size, b) | ||
| } | ||
|
|
||
| func (a *alignedAllocator) Free(b []byte) { | ||
| a.mem.Free(b) | ||
| } |
There was a problem hiding this comment.
probably simpler to just embed the allocator instead of having to recreate these:
type alignedAllocator struct {
Allocator
}and creating it would be:
return &alignedAllocator{Allocator: mem}that way it just picks up the embedded methods instead of having to write them ourselves. If we ever change anything about the Allocator interface we won't have to also make a change here.
| next := roundUpToMultipleOf64(addr) | ||
| if addr != next { | ||
| shift := next - addr | ||
| return buf[shift : size+shift : size+shift], buf |
There was a problem hiding this comment.
shouldn't the capacity be just size?
zeroshade
changed the title
Ah....That is an option, but yikes 😅 |
|
But yeah, unless we do that we have to somehow keep the "real" allocation around since the allocator interface works in term of |
Rationale for this change
We pay a bit of up-front memory to make sure allocations are always aligned, making in-process FFI easier.
What changes are included in this PR?
Always align in Buffer.
Are these changes tested?
Yes
Are there any user-facing changes?
~Maybe: the default Go allocator already aligned allocations. Other allocators may now have a bit more overhead. We will now effectively never use
reallocwith the mallocator, instead always preferring a freshmalloc+memcpy+free.Closes #282.