Repository navigation
perf(query): skip per-node memclr in newNode (Allocate vs AllocateAligned) - #9711
shaunpatterson wants to merge 1 commit into
Conversation
337203e to
7ecf7f3
Compare
…gned)
z.Allocator.AllocateAligned zeros every byte of its result via ZeroOut.
For encoder.newNode this shows up as runtime.memclrNoHeapPointers and
accounted for ~66% of CPU in BenchmarkFastJsonNode2Chilren — millions
of 24-byte memsets per query, all writing zero into memory that was
already zero.
The buffers behind z.Allocator come from z.Calloc, which is
zero-initialized by libc. Each encoder gets a freshly created
z.Allocator (z.NewAllocator(4<<10, ...) in newEncoder) and the
allocator is a strict-forward bump allocator, so no slot is ever
re-handed-out within the encoder's lifetime. Switching from
AllocateAligned to plain Allocate is therefore safe.
For the manual alignment loop AllocateAligned does on top of memclr,
we replace it with a static invariant: nodeSize % nodeAlign == 0,
enforced in init() with a panic. With nodeSize=24 and nodeAlign=8 on
64-bit (and equivalent values on 32-bit), consecutive 24-byte slices
out of a base-aligned buffer remain 8-byte aligned, so the explicit
alignment fixup is unnecessary.
GC safety: node lives in []byte slabs from z.Calloc (cgo-backed,
off-heap); Go's GC doesn't scan those, and the *node pointers inside
each node only point into the same allocator.
Perf (Apple M4 Max, 5x3s avg):
BenchmarkFastJsonNode2Chilren 86.58M -> 75.86M ns/op (-12.4%)
(-13.9% vs baseline)
BenchmarkFastJsonNodeEmpty 16.97M -> 9.14M ns/op (-46.1%)
7ecf7f3 to
79cb0c3
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: replacing AllocateAligned with Allocate in newNode to skip per-node memclr for performance. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
| // hotspot (66% of CPU in BenchmarkFastJsonNode2Chilren). | ||
| func (enc *encoder) newNode(attr uint16) fastJsonNode { | ||
| b := enc.alloc.AllocateAligned(nodeSize) | ||
| b := enc.alloc.Allocate(nodeSize) |
There was a problem hiding this comment.
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.allochas to be exactlynodeSize. If someone later usesenc.alloc.Copy(val)orAllocate(n)for scalar values, the nodes after it are no longer 8-byte aligned. amd64/arm64 won't care, but checkptr under-racewill panic on the(*node)(unsafe.Pointer(...))conversion. - The allocator can never be reused. That's why
AllocateAlignedzeroes in the first place (see the "We are reusing allocators" comment in ristretto'sz/allocator.go). IfnewEncoderever moves toz.AllocatorPoolorReset(), nodes come back with stalenext/childpointers. 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.
| // 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() { |
There was a problem hiding this comment.
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).
2 participants
Description
z.Allocator.AllocateAlignedzeros every byte of its result viaZeroOut. Forencoder.newNodethis shows up asruntime.memclrNoHeapPointersand accounted for ~66% of CPU inBenchmarkFastJsonNode2Chilren— millions of 24-byte memsets per query, all writing zero into memory that was already zero.The buffers behind
z.Allocatorcome fromz.Calloc, which is zero-initialized by libc. Each encoder gets a freshly createdz.Allocator(z.NewAllocator(4<<10, ...)innewEncoder) and the allocator is a strict-forward bump allocator, so no slot is ever re-handed-out within the encoder's lifetime. Switching fromAllocateAlignedto plainAllocateis therefore safe.For the manual alignment loop
AllocateAligneddoes on top ofmemclr, we replace it with a static invariant:nodeSize % nodeAlign == 0, enforced ininit()with a panic. WithnodeSize=24andnodeAlign=8on 64-bit (and equivalent values on 32-bit), consecutive 24-byte slices out of a base-aligned buffer remain 8-byte aligned, so the explicit alignment fixup is unnecessary.GC safety:
nodelives in[]byteslabs fromz.Calloc(cgo-backed, off-heap); Go's GC doesn't scan those, and the*nodepointers inside each node only point into the same allocator.Tests added:
TestNewNodeZeroInit: allocates 4096 nodes and verifies that for every node,getAttrreturns the seed attr andnext/childare nil — i.e., the underlying memory was zero before we wrote to it.TestNewNodeAlignment: allocates 1024 nodes and asserts the*nodepointer isnodeAlign-aligned every time.Perf (Apple M4 Max, 5×3s avg):
BenchmarkFastJsonNode2Chilren: 86.58M → 75.86M ns/op (−12.4%, −13.9% vs baseline)BenchmarkFastJsonNodeEmpty: 16.97M → 9.14M ns/op (−46.1%)Checklist
fix:,feat:,chore:,ci:, etc.Thank you for your contribution to Dgraph!
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Compatibility