zeroshade commented on code in PR #1132:
URL: https://github.com/apache/arrow-go/pull/1132#discussion_r3752474027
##########
arrow/memory/checked_allocator.go:
##########
@@ -68,14 +68,22 @@ func (a *CheckedAllocator) Allocate(size int) []byte {
func (a *CheckedAllocator) Reallocate(size int, b []byte) []byte {
a.sz.Add(int64(size - len(b)))
- oldptr := uintptr(unsafe.Pointer(&b[0]))
+ var oldptr uintptr
+ if cap(b) > 0 {
+ oldptr = uintptr(unsafe.Pointer(&b[:1][0]))
Review Comment:
`&b[:1][0]` works, but it's a reslice trick that makes a reader stop and
work out why it doesn't panic. `unsafe.SliceData` states the intent directly,
is already used in this repo (`internal/utils/min_max_neon_arm64.go` and
friends), and the `go 1.25.0` baseline has it.
```suggestion
oldptr = uintptr(unsafe.Pointer(unsafe.SliceData(b)))
```
Keep the `cap(b) > 0` guard either way — for a non-nil zero-capacity slice
`SliceData` returns an unspecified non-nil pointer, so the guard is still doing
real work.
##########
arrow/memory/checked_allocator.go:
##########
@@ -68,14 +68,22 @@ func (a *CheckedAllocator) Allocate(size int) []byte {
func (a *CheckedAllocator) Reallocate(size int, b []byte) []byte {
a.sz.Add(int64(size - len(b)))
- oldptr := uintptr(unsafe.Pointer(&b[0]))
+ var oldptr uintptr
+ if cap(b) > 0 {
+ oldptr = uintptr(unsafe.Pointer(&b[:1][0]))
+ }
out := a.mem.Reallocate(size, b)
if size == 0 {
+ if oldptr != 0 {
+ a.allocs.Delete(oldptr)
+ }
return out
}
newptr := uintptr(unsafe.Pointer(&out[0]))
- a.allocs.Delete(oldptr)
+ if oldptr != 0 {
+ a.allocs.Delete(oldptr)
+ }
Review Comment:
The delete now happens on both paths, so it can hoist above the `size == 0`
check and stop being duplicated:
```suggestion
out := a.mem.Reallocate(size, b)
if oldptr != 0 {
a.allocs.Delete(oldptr)
}
if size == 0 {
return out
}
newptr := uintptr(unsafe.Pointer(&out[0]))
```
Exactly equivalent, one copy instead of two.
##########
arrow/memory/buffer_test.go:
##########
@@ -58,6 +58,17 @@ func TestNewResizableBuffer(t *testing.T) {
assert.Zero(t, buf.Len())
}
+func TestCheckedAllocatorReallocate(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.NewGoAllocator())
+ defer mem.AssertSize(t, 0)
+
+ buf := mem.Reallocate(16, nil)
+ assert.Len(t, buf, 16)
+
+ buf = mem.Reallocate(0, buf)
+ assert.Empty(t, buf)
Review Comment:
Coverage gap, and a question that comes with it.
These cases cover `Reallocate(16, nil)` (cap 0) and `Reallocate(0, buf)`,
but not **len 0 with cap > 0** — which is precisely the case the `cap(b) > 0`
guard exists for, since plain `&b[0]` panics there too.
Before adding that case, though, it's worth checking whether such a slice
can actually reach here, because the guard and the accounting disagree. The
guard keys off `cap(b)`, but the first line of the function does:
```go
a.sz.Add(int64(size - len(b)))
```
which keys off `len(b)`. If a slice with `len 0, cap 16` arrives, the
pointer handling is now correct but `sz` under-reports the old allocation by 16
bytes, so the tracked total drifts and `AssertSize` fails for a different
reason. I tried writing the obvious test for this and it fails on the
accounting, not the pointer.
So I think it's one of two things:
- such slices can't reach `Reallocate`, in which case `cap(b) > 0` could
just as well be `len(b) > 0` and the intent is clearer; or
- they can, in which case the accounting needs the same treatment and
there's a second bug here.
No need to solve it in this PR either way — but the answer decides whether
the extra test case is worth adding now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
