[euank]

The mutual recursion between insertNode and insertRecord resulted in a lot of unique and fairly deep call stacks. That's normally fine, except if you use the Go heap profiler, that ends up chewing through a lot of memory.

See golang/go#69590 for discussion about why having a lot of unique stacks results in high memory usage.

This PR rewrites the mutual recursion into iteration since any recursion can be implemented iteratively by simply keeping track of the stack yourself, and that keeps the go heap profiler happier.

As you can see from the commit co-author, this was AI assisted, but I also stand by this code as a human (i.e. I have reviewed it and am happy with it).

Summary by CodeRabbit

• Reliability
  • Network insertions now use a fixed-depth traversal, including when processing deeply nested IPv6 prefixes.
  • If an insertion encounters an error, remaining child traversal is skipped while records are still merged during the return path.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: b12603de-99f7-4409-af97-0e86ffaf50d6

📥 Commits

Reviewing files that changed from the base of the PR and between 9a8f309 and 1fc6a9f.

📒 Files selected for processing (3)

• node.go
• node_test.go
• tree.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

Insertion now traverses nodes with an explicit stack instead of recursive child visits. The traversal tracks child selection, descent, and errors, and merges records while unwinding. A new test checks callback stack depth for IPv6 prefix insertions.

Changes: Iterative insertion

Layer / File(s)	Summary
Stacked insertion traversal node.go, tree.go, node_test.go	insertNode and insertRecord delegate traversal to insertWalk, which uses the tree’s fixed-size stack. insertAt returns descent status with any error. The test checks callback stack depth across IPv6 prefix insertions.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant insertNode
  participant insertWalk
  participant insertAt
  insertNode->>insertWalk: Start stack-based traversal
  insertWalk->>insertAt: Process current record
  insertAt-->>insertWalk: Return descend status and error
  insertWalk->>insertWalk: Visit selected children and merge records while unwinding

Loading

Suggested reviewers: oschwald

Merge Risk: ⚪ Minimal · up to 1fc6a

The change makes insertion iterative to avoid deep recursion and heap-profiler memory growth. No concrete merge-blocking risk was identified in the supplied context.

Pre-merge checks · 5

Passed checks (5 passed)

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 recursive insert walks with iterative traversal.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.
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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit watched the stack take shape,
As prefixes crossed the IPv6 landscape.
It hopped through branches, left and right,
Then merged the records out of sight.
“No deep call stack,” it cheered with delight!}

_{Comment @coderabbitai help to get the list of available commands.}

[oschwald]

This comment is from Claude Code, an AI agent, posted on behalf of @oschwald.

I measured the impact of this PR at 1fc6a9f against origin/main (9a8f309). Setup: Go 1.27.1, i7-13700K, benchmarks pinned to 4 cores, run in alternating order (main, PR, PR, main), 8 runs per side unless noted.

Heap profiler

The claim holds. Test workload: load GeoLite2-City, overlay each GeoLite2-ASN network with inserter.DeepMerge, insert a cover value on each IPv4 /8, then WriteTo. I counted buckets with runtime.MemProfile and read /memory/classes/profiling/buckets:bytes.

MemProfileRate	main	PR
512 KiB (default)	5,444 buckets, 4.1 MB	285 buckets, 1.5 MB
4096	410,562 buckets, 219 MB	683 buckets, 1.6 MB

On main, each insert pass adds buckets, and the runtime never frees them. The new TestInsertCallbackStackDepthIsConstant fails on main (111 distinct stack depths) and passes on this branch.

This only affects programs that link runtime.MemProfile, for example through net/http/pprof. Otherwise the linker turns memory profiling off.

Correctness

• The full test suite passes, also with -race.
• The workload above writes the same 145 MB database on main and on this branch. The only byte that differs is build_epoch in the metadata.
• Error handling matches the recursive version. After an error, the walk skips the remaining children and still merges every node on the stack.

Performance

Time per operation. A plus sign means slower.

Benchmark	main	PR	Change
TreeInsertDeepMergeFragmentedNetwork	135.8 µs	166.4 µs	+22.6% (p=0.000)
TreeInsertAddressOrder (12 cases)			0% to +2.5%, 9 of 12 significant
TreeInsertOverlappingPasses	3.857 ms	3.922 ms	+1.7%
TreeInsertTopLevelMergeOverlappingPasses	2.856 ms	2.897 ms	+1.4%
TreeInsertRangeFragmentedPasses	596.7 µs	609.5 µs	+2.2%
TreeInsertChurnRepeatedPasses	548.0 µs	555.0 µs	+1.3%
EnterpriseLoadThenOverlay	51.69 ms	51.26 ms	no significant change
TreeInsertEnterpriseDedupRates (4 cases)			no change, except 99% hits at -1.0%
TreeLoadExternalMMDB, GeoLite2-City	1.757 s	1.797 s	+2.25% (p=0.028, spread ±8%)
TreeLoadExternalMMDB, GeoIP2-Enterprise	11.60 s	11.85 s	no significant change (n=4)

The end-to-end workload above, with profiling off, gave the same time on both sides. Every phase (load, overlay, cover, write) was within ±1.5% of main over 6 runs per side.

TreeInsertDeepMergeFragmentedNetwork times one /20 insert over 4,096 existing /32 records with a memoized inserter. Most of that time is the walk that this PR rewrote. The regression is about 2.5 ns for each visited record. In the real-database runs, value-store and callback work hide this cost.

Allocations: B/op goes up about 4 KiB for each new Tree, because of insertStack. allocs/op does not change.

I tried 2 changes to recover the walk cost. Neither helped:

• Pointer-free 8-byte frames, to remove the GC write barrier on push: +25.6% against main.
• nextChild inlined by hand (it costs 143, over the inline budget of 80): +21.7% against main.

I did not find the cause of the remaining walk cost.

[oschwald]

Before this gets merged, it would be worth trying to determine if we could eliminate the overhead of the change. Did you see a more significant impact when building databases yourself? The PR description doesn't include any observed impact.

[euank]

Thanks for the review, appreciated!

I spent some time poking at the overhead, but I think it's mostly just inherent. The call stack is a more optimized path than a hand-written userspace stack, so even if we're doing something fairly similar, it takes more total instructions and has a little worse performance on these benchmarks.

Did you see a more significant impact when building databases yourself?

Our use of mmdb isn't super sensitive to CPU performance (since the rest of our code's CPU usage dwarfs mmdb), so we don't notice any holistic performance issues with this.

For our use-case, however, the memory usage is really noticeable and we see periodic OOM kills without this change.