From ccd6b4abb4e7d5c05249d617aac01596d0fc981b Mon Sep 17 00:00:00 2001 From: Francis Stephens Date: Fri, 17 Jan 2025 19:39:48 +1300 Subject: [PATCH 1/3] Adding gcassert:noescape to all RefPointer methods Each of these methods have a pointer receiver. But RefPointer is used as a value, so it's important that the receiver does not escape to the heap. If it does escape then each method call(!) will allocate to the heap. As a side note, the rationale behind using a pointer receiver is to avoid copying two words on RefPointer method calls. This rationale makes sense, but hasn't been tested as a performance improvement. In the future we should test this. Maybe value receivers are just as good (or better). --- .../pointerstore/pointer_reference.go | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/offheap/internal/pointerstore/pointer_reference.go b/offheap/internal/pointerstore/pointer_reference.go index 3692651..7b13d1b 100644 --- a/offheap/internal/pointerstore/pointer_reference.go +++ b/offheap/internal/pointerstore/pointer_reference.go @@ -13,12 +13,23 @@ const maskShift = 56 // This leaves 8 bits for the generation data const genMask = uint64(0xFF << maskShift) const pointerMask = ^genMask -// The address field holds a pointer to an object, but also sneaks a -// generation value in the top 8 bits of the metaAddress field. +// The address field holds a pointer to an object, but also sneaks a generation +// value in the top 8 bits of the metaAddress field. // // The generation must be masked out to get a usable pointer value. The object // pointed to must have the same generation value in order to access/free that // object. +// +// Because the Refpointer struct is two words (on 64 bit systems) in size, all +// method receivers are pointers to avoid copying two words in method calls. +// However, the RefPointer is _always_ used as a value. This means that having +// a pointer receiver could potentially cause the RefPointer to be allocated if +// its receiver escapes to the heap (according to escape analysis). We assert +// that all methods do not allow their receiver variable to escape to the heap. +// +// This is all very nice and good, but the idea that avoiding copying two words +// is a meaningful improvement are speculative and haven't been tested. This +// would be a good target for future performance testing. type RefPointer struct { dataAddress uint64 metaAddress uint64 @@ -60,6 +71,7 @@ func NewReference(pAddress, pMetadata uintptr) RefPointer { } } +//gcassert:noescape func (r *RefPointer) AllocFromFree() (nextFree RefPointer) { // Grab the nextFree reference, and nil it for this metadata meta := r.metadata() @@ -80,6 +92,7 @@ func (r *RefPointer) AllocFromFree() (nextFree RefPointer) { return nextFree } +//gcassert:noescape func (r *RefPointer) Free(oldFree RefPointer) { meta := r.metadata() @@ -98,10 +111,12 @@ func (r *RefPointer) Free(oldFree RefPointer) { } } +//gcassert:noescape func (r *RefPointer) IsNil() bool { return r.metadataPtr() == 0 } +//gcassert:noescape func (r *RefPointer) DataPtr() uintptr { meta := r.metadata() @@ -123,23 +138,29 @@ func (r *RefPointer) DataPtr() uintptr { } // Convenient method to retrieve raw data of an allocation +// +//gcassert:noescape func (r *RefPointer) Bytes(size int) []byte { ptr := r.DataPtr() return pointerToBytes(ptr, size) } +//gcassert:noescape func (r *RefPointer) metadataPtr() uintptr { return (uintptr)(r.metaAddress & pointerMask) } +//gcassert:noescape func (r *RefPointer) metadata() *metadata { return (*metadata)(unsafe.Pointer(r.metadataPtr())) } +//gcassert:noescape func (r *RefPointer) Gen() uint8 { return (uint8)((r.metaAddress & genMask) >> maskShift) } +//gcassert:noescape func (r *RefPointer) setGen(gen uint8) { r.metaAddress = (r.metaAddress & pointerMask) | (uint64(gen) << maskShift) } From 832f57622f95895fbfa4923f9224508a347d0849 Mon Sep 17 00:00:00 2001 From: Francis Stephens Date: Fri, 17 Jan 2025 23:30:47 +1300 Subject: [PATCH 2/3] Switch to fmstephe fork of gcassert At this time this fork of gcassert detects leaking parameter messages indicating that a parameter has escaped to the heap. Remove gcassert if it exists rm rm Use version v0.0.3 of my gcassert fork --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 8694f55..342ed8f 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -29,7 +29,7 @@ jobs: - name: Run gcassert run: | - go install github.com/jordanlewis/gcassert/cmd/gcassert@latest + go install github.com/fmstephe/gcassert/cmd/gcassert@v0.0.3 $(go env GOPATH)/bin/gcassert ./... - name: Check code quality From 3d6274176d20a6b174779de62864fc622b76f591 Mon Sep 17 00:00:00 2001 From: Francis Stephens Date: Sat, 18 Jan 2025 00:39:42 +1300 Subject: [PATCH 3/3] Avoid RefPointer.Free() leaking its handler Because the handler is passed into a fmt.Errorf(...) call on an error path the handler is forced to be on the heap. We take the value of the handler to avoid this. This error was picked up by the gcassert:noescape annotation. Very satisfying. --- offheap/internal/pointerstore/pointer_reference.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/offheap/internal/pointerstore/pointer_reference.go b/offheap/internal/pointerstore/pointer_reference.go index 7b13d1b..8fa0fbd 100644 --- a/offheap/internal/pointerstore/pointer_reference.go +++ b/offheap/internal/pointerstore/pointer_reference.go @@ -97,7 +97,9 @@ func (r *RefPointer) Free(oldFree RefPointer) { meta := r.metadata() if !meta.nextFree.IsNil() { - panic(fmt.Errorf("attempted to Free freed allocation %v", r)) + // NB: The odd-looking *r here actually prevents an allocation. + // Fuller explanation found in DataPtr() + panic(fmt.Errorf("attempted to Free freed allocation %v", *r)) } if meta.gen != r.Gen() {