pixelherodev commented on PR #322:
URL: https://github.com/apache/arrow-go/pull/322#issuecomment-3564556566
There's currently 121 different implementations of Release, and 56 of
Retain. A _lot_ of them are just this shell:
```go
func (t *Type) Release() {
debug.Assert(t.refCount.Load() > 0, "too many releases")
if t.refCount.Add(-1) == 0 {
if t.someSubfield != nil {
t.someSubfield.Release()
t.someSubfield = nil
}
}
}
```
[not to mention that that debug assertion is still doing an extra atomic per
release; the Load() can't be optimized out. That said, on x64 at least, it's
just
```
XCHGL AX, AX
MOVQ 16(AX), CX
```
, hardly a big deal.]
Realistically, these are just wrappers of the subfields. A lot of these are
allocated on the Go heap, referencing Allocator memory as a subfield. What
we're basically saying is "the subfield's memory is valid for as long as the
field itself is."
...except that the field will actually outlive the subfield, so we nil out
the reference to the allocated memory, because the field itself is on the Go
heap.
These shells could all become
```go
func (t *Type) Release() {
t.someSubfield.Release()
}
```
without issue IMO, and the same change made to the Retain. The
Retain/Release is really tracking how many additional references to the
_subfield_ are present.
What we really have is _some_ allocations that need managed explicitly, and
some Go-heap-allocated memory which _references it_ in a dependency tree.
e.g. ipc/MessageReader references ipc/Message, which holds two actual
Buffers that need to be freed.
```go
type Allocation struct {
allocator *mem.Allocator
data []byte
}
type Refcount struct {
dependencies []*Refcount
memories []*Allocation
}
```
We could then e.g. have Buffer embed an Allocation at the root so that it
can be used as one, instead of storing the two pointers directly; then, it'd
embed a Refcount which points at that allocation - and any child buffer would
have a Refcount with a dependency of the parent.
Then instead of each type implementing the release logic manually, it would
set up the dependencies and `.Release()` would be a single implementation that
would walk the dependencies?
--
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]
