Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions query/outputnode.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,19 @@ type node struct {
child *node
}

var nodeSize = int(unsafe.Sizeof(node{}))
var (
nodeSize = int(unsafe.Sizeof(node{}))
nodeAlign = int(unsafe.Alignof(node{}))
)

// init enforces that nodeSize is a multiple of nodeAlign so that consecutive
// bump-allocations from a base-aligned buffer all return aligned slices. This
// is what makes the AllocateAligned-free fast path in newNode safe.
func init() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't fire. Go always rounds a struct's size up to a multiple of its alignment (arrays depend on it), so nodeSize % nodeAlign is always 0. I'd drop the init() and nodeAlign. The rule that can actually break is that every allocation from enc.alloc is nodeSize bytes (see my comment on newNode).

if nodeSize%nodeAlign != 0 {
panic("query: nodeSize must be a multiple of nodeAlign")
}
}

func newEncoder() *encoder {
idSlice := make([]string, 1)
Expand Down Expand Up @@ -300,8 +312,18 @@ type fastJsonNode *node

// newNode returns a fastJsonNode with its attr set to attr,
// and all other meta set to their default value.
//
// We use Allocate (no zeroing) rather than AllocateAligned because:
// 1. The underlying allocator buffers come from z.Calloc and are zero-initialized.
// 2. The allocator is freshly created per encoder (not pooled) and never reuses
// space within its lifetime (Allocate is a strict-forward bump allocator).
// 3. nodeSize (24 on 64-bit) is a multiple of nodeAlign — see init() — so
// consecutive Allocate(nodeSize) calls preserve alignment.
//
// Skipping the per-node memclr eliminates the runtime.memclrNoHeapPointers
// hotspot (66% of CPU in BenchmarkFastJsonNode2Chilren).
func (enc *encoder) newNode(attr uint16) fastJsonNode {
b := enc.alloc.AllocateAligned(nodeSize)
b := enc.alloc.Allocate(nodeSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is safe today, but it now leans on two rules that nothing enforces, and the comment on newNode isn't where someone changing them would look:

  • Every allocation from enc.alloc has to be exactly nodeSize. If someone later uses enc.alloc.Copy(val) or Allocate(n) for scalar values, the nodes after it are no longer 8-byte aligned. amd64/arm64 won't care, but checkptr under -race will panic on the (*node)(unsafe.Pointer(...)) conversion.
  • The allocator can never be reused. That's why AllocateAligned zeroes in the first place (see the "We are reusing allocators" comment in ristretto's z/allocator.go). If newEncoder ever moves to z.AllocatorPool or Reset(), nodes come back with stale next/child pointers. That shows up as corrupt JSON or pointer cycles, not a clean crash.

TestNewNodeZeroInit wouldn't catch the second case, since the first Get on an empty pool also hands back a fresh, zeroed allocator. Could you move this contract onto the alloc field in encoder? That's where a pooling or reuse change would land.

n := (*node)(unsafe.Pointer(&b[0]))
enc.setAttr(n, attr)
return n
Expand Down
39 changes: 39 additions & 0 deletions query/outputnode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"sync"
"testing"
"time"
"unsafe"

"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -166,6 +167,44 @@ func TestFastJsonNode(t *testing.T) {
require.Equal(t, fj2, enc.children(fj))
}

// TestNewNodeZeroInit guards the safety invariant for newNode's use of
// z.Allocator.Allocate (which does not zero memory) instead of
// AllocateAligned. The allocator is backed by z.Calloc-initialized buffers
// that come up zero, and the bump allocator never re-hands-out a slot
// within an encoder's lifetime, so every node returned by newNode must
// have only the attr bits set in meta and zero next/child pointers.
func TestNewNodeZeroInit(t *testing.T) {
enc := newEncoder()
const N = 4096
for i := 1; i <= N; i++ {
attr := uint16(i)
fj := enc.newNode(attr)
require.Equal(t, attr, enc.getAttr(fj),
"newNode #%d: attr round-trip", i)
require.Equal(t, uint64(attr)<<40, fj.meta,
"newNode #%d: meta must contain only attr bits, no leftover state", i)
require.Nil(t, fj.next,
"newNode #%d: next must be nil for a freshly allocated node", i)
require.Nil(t, fj.child,
"newNode #%d: child must be nil for a freshly allocated node", i)
}
}

// TestNewNodeAlignment guards the alignment invariant that lets newNode
// skip AllocateAligned: every node returned from a sequence of newNode
// calls must satisfy unsafe.Alignof(node{}), so subsequent typed pointer
// access is safe.
func TestNewNodeAlignment(t *testing.T) {
enc := newEncoder()
want := uintptr(unsafe.Alignof(node{}))
for i := 0; i < 1024; i++ {
fj := enc.newNode(uint16(i + 1))
addr := uintptr(unsafe.Pointer(fj))
require.Zerof(t, addr&(want-1),
"node #%d at %x is not %d-byte aligned", i, addr, want)
}
}

func BenchmarkFastJsonNodeEmpty(b *testing.B) {
for i := 0; i < b.N; i++ {
enc := newEncoder()
Expand Down
Loading