proof of concept: unify refcounting logic

[pixelherodev]

A step towards the addcleanup work: we want to be able to turn refcounting off entirely. Rather than move all ~200 release+retain functions into files with build tags, IMO it makes sense to unify all refcounting logic into memory.Refcount, embed that everywhere else, and then we can just turn that off with switching to AddCleanup.

This is a proof of concept using that with just ipc/Message to obtain feedback before finishing the work.

[pixelherodev]

Tests are passing.

The usage of Additional for nilling derived pointers is because those can be of arbitrary types, and I didn't want to try engaging in any or unsafe.Pointer shenanigans to construct the equivalent to a C void**.

[pixelherodev]

... ah wait

The usage of Additional for nilling derived pointers is because those can be of arbitrary types, and I didn't want to try engaging in any or unsafe.Pointer shenanigans to construct the equivalent to a C void**.

... should be able to recast them all as *uintptr with unsafe.Pointer, and then store it as []*uintptr? 🤔

[pixelherodev]

Tested that the derived pointers are nilled correctly by modifying the MessageReader test, definitely is working.

It's also guaranteed to be safe:

(1) Conversion of a *T1 to Pointer to *T2.

Provided that T2 is no larger than T1 and that the two share an equivalent
memory layout, this conversion allows reinterpreting data of one type as
data of another type. An example is the implementation of math.Float64bits:

[pixelherodev]

In principle, doing this and then moving Refcount.(Retain|Release) into a disabled file is enough to switch off refcounting.

I'd also like to see if we can move the Refcount initialization into an inline-able function call, though - that would preserve this behavior when it's enabled, but also drop the cost of initializing the refcount information when it's not needed...

[pixelherodev]

And, tested and confirmed: this prototype completely drops refcounting from the Message type when the refcount build flag is not enabled, including making it zero-size so that the Go heap shrinks a bit too :)

[pixelherodev]

And, tested with MessageReader dependency on Message. Test passes, and if the explicit dependency is dropped, the test fails due to a memory leak.

[pixelherodev]

There's probably a way to replace the unsafe.Pointer in storage for dependencies with **Refcount, at least. I'm not sure I like this approach; it forces dependencies to have a pointer stored, which effectively forces heap escapes, which I've been trying to avoid.

Might be worth splitting optional/mutable/dynamic dependencies (which need to be able to have the pointer change) and immutable/static dependencies, waiting on review before I make any more changes though

[pixelherodev]

go fmted. Forgot 🙃

[zeroshade]

I'm not a fan of this approach. I dislike the users having to call multiple functions as opposed to just having Retain and Release (i.e. I don't like having to call the Referenced and keep track of Derived etc.)

In theory, a buffer need only keep track of its parent (if it has one) and doesn't need to keep track of any other buffers which were sliced off from it. I don't understand the need/desire for adding the pointers that you're doing as opposed to just having the atomic.Int64 and essentially putting that behind a struct which can have a version that is empty for turning off the refcounts.

i.e. Why isn't it just something like:

//go:build !norc

type RefCount struct {
    ref atomic.Int64
    cleanup func()
}

func (r *RefCount) Retain() {
    ...
}

func (r *RefCount) Release() {
    ...
}

/////////////////////
//go:build norc

type RefCount struct {}
func (*RefCount) Retain() {}
func (*RefCount) Release() {}

[zeroshade]

we shouldn't need to track the m.msg, the point of the refcounting is to track allocations made by the memory.Allocator, the flatbuffer message object is never allocated by the memory.Allocator

[pixelherodev]

The msg pointer is derived from the meta Buffer, which is itself allocated by the Allocator. ReferenceDerived is not for refcounting; it basically means "when this object goes free, nil out the target to prevent use-after-free."

Without it, accessing the message after calling Release could potentially access other memory allocated by the Allocator - if integrating with C, this could theoretically allow access to other heap allocations. Note that the previous code for Message.Release contains this:

if msg.refCount.Add(-1) == 0 {
		msg.meta.Release()
		msg.body.Release()
		msg.msg = nil
		msg.meta = nil
		msg.body = nil
	}

it's releasing the two subresources, but also nilling the one that depends on one. This is just to replace the msg = nil line, basically.

[zeroshade]

why **Buffer instead of *Buffer?

[pixelherodev]

I'll explain in a dedicated comment going over the whole design

[zeroshade]

you mean "When this object is cleaned up" or "released"? Because Go is garbage collected and we don't have destructors, then just being unreferenced won't deallocate the buffers immediately, they'll get deallocated when the GC gets around to it

[pixelherodev]

no, this is for the last call to Refcount.Release(): that call will deallocate the buffers immediately - or, rather, it will call Allocator.Free() immediately. IMO these should be seen as the same thing.

[pixelherodev]

I'm just going to write up the design notes as one place to explain all the weirdness:

• Looking over the existing code for Message/MessageReader, there's a few requirements:
  • Buffers get released when the last reference to them is erased.
  • When buffers get released, we also nil out the pointer to them.
  • When 1 object is released, we want to walk every object it references and release those too.
    • This is often done by value at the time of Release, not when the object is created, and includes a "if != nil" check. The object may not be created until after the object is initialized, if ever, and should be released regardless. e.g. MessageReader.msg is a *Message, and has a different value for each Message that is read.

So, in trying to separate the concerns of reference counting and object management, and trying to make it all declarative and only called on object initialization - keeping the refcount graph static, even as the objects in it may be dynamic! - I settled on using two-level-pointers. The address of MessageReader.msg is fixed, even as its value may change - we want to release whatever the final value is when MessageReader gets released, and we don't want to insert a lot more code dynamically shifting around the reference graph.

[pixelherodev]

    cleanup func()

Using a cleanup function (or closure) may well be the best approach. I can play around with that instead and see if we like that more? It means not needing to manually track the graph at all, because the function is invoked on release time but defined at creation time; it can take a closure of the object itself, and check whether fields needs unreferenced without any of the typing shenanigans..

[pixelherodev]

    cleanup func()

Using a cleanup function (or closure) may well be the best approach. I can play around with that instead and see if we like that more? It means not needing to manually track the graph at all, because the function is invoked on release time but defined at creation time; it can take a closure of the object itself, and check whether fields needs unreferenced without any of the typing shenanigans..

One worry I have: I'll need to check if the compiler can optimize out the construction of the closure, since it won't actually be used. The current approach generates zero code when disabled, I want to make sure we can maintain that property...

[zeroshade]

That's a good thing to check. My opinion here is the desire to make things as simple as possible and avoid having to deal with the dependency graph like that.

In addition, the closure approach would also make the transition to add cleanup much easier

[pixelherodev]

That's a good thing to check. My opinion here is the desire to make things as simple as possible and avoid having to deal with the dependency graph like that.

Agreed. It's likely worth it even if it does; more overhead than this approach maybe, but less code / correctness concerns, and still a savings over the current implementation :)

In addition, the closure approach would also make the transition to add cleanup much easier

Yeah, that's a really good point. We'd basically need two flags - the question is, is it one for "do any tracking" and one for "refcount vs addcleanup", or is it one for refcounting and one for addcleanup, and the combination is invalid?