Skip to content

zipper: pool recursive root builders in write path#820

Open
snissn wants to merge 3 commits intopr/batch-arena-reserve-tuningfrom
pr/zipper-scratch-reuse-tuning
Open

zipper: pool recursive root builders in write path#820
snissn wants to merge 3 commits intopr/batch-arena-reserve-tuningfrom
pr/zipper-scratch-reuse-tuning

Conversation

@snissn
Copy link
Copy Markdown
Owner

@snissn snissn commented Mar 13, 2026

Summary

  • pool root builders in writeRecursive for both leaf and internal paths
  • re-use existing builder pool/reset flow already used by split builders
  • keep builder scratch release semantics unchanged (ReleaseScratch before re-pool)

Why

batch_write_steady alloc profiles still showed zipper write-path wrapper churn, including node.NewBuilderWithOptions under zipper.(*Zipper).writeRecursive.

Validation

  • go test ./TreeDB/zipper ./TreeDB/caching
  • time ./bin/unified-bench -dbs treedb -profile fast -keys 500000 -progress=false -treedb-index-outer-leaves-in-vlog=true -valsize=100 -treedb-force-value-pointers=false -checkpoint-between-tests -profile-dir=/home/mikers/tmp/perf-zipper-builderpool-b-1773435791

Comparison baseline (PR #819 head): /home/mikers/tmp/perf-batch-geogrow-b-1773435448

alloc_space (batch_write_steady)

  • total: 122.66MB -> 111MB
  • zipper.(*Zipper).writeRecursive: 56.22MB -> 53.71MB
  • node.NewBuilderWithOptions: 8.50MB -> ~0MB

ops/sec (selected)

  • batch_write_steady: 2,231,000 -> 2,239,040 (+0.4%)
  • batch_write: 12,610,767 -> 12,496,662 (-0.9%)
  • batch_random: 8,278,773 -> 8,207,744 (-0.9%)
  • dataset_write_sorted: 3,634,457 -> 3,726,001 (+2.5%)

Copilot AI review requested due to automatic review settings March 13, 2026 21:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors Zipper’s page-builder allocation to reuse node.Builder instances via a shared pool, reducing per-merge allocations on the write path.

Changes:

  • Introduces newPooledBuilderForType / releasePooledBuilder to pool builders for both leaf and internal pages.
  • Switches writeRecursive to use pooled builders for leaf and internal merges (with deferred release).
  • Keeps releasePooledLeafBuilder as a thin wrapper for existing call sites.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors Zipper’s page-builder allocation to use pooled node.Builder instances more broadly, reducing per-merge allocations during recursive writes.

Changes:

  • Simplified newPooledLeafBuilder by delegating to a new generalized newPooledBuilderForType.
  • Introduced releasePooledBuilder and updated call sites to return builders to the pool in writeRecursive.
  • Added support for pooled builder creation for internal pages (including InternalBaseDelta option propagation).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants