feat: improve arena implementation #12
Conversation
WalkthroughDeleted the internal Arena and pooling code, integrated external github.com/wundergraph/go-arena, threaded arena through parsing, merging, updating, and utility APIs, added arena-backed Value constructors, removed related tests/benchmarks, and bumped Go/tooling and CI dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser.go (1)
349-433: Add nil-arena guard in unescapeStringBestEffort
AllocateSlice and SliceAppend require a non-nil *Arena (passing nil panics). Branch ona == nilto use built-inmake/appendfor heap fallback (or introduce helper wrappers likeMakeSliceOrHeap/AppendOrHeap).
🧹 Nitpick comments (1)
parser.go (1)
596-599: Optional: arena-aware slow paths for Get/Visit.Get/Visit unescape with nil arena, which may allocate on the heap and add GC pressure. If these are hot in arena-backed flows, consider arena-aware variants (e.g., GetWithArena/VisitWithArena) or plumbing arena into these calls.
Also applies to: 616-618
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
arena.go(0 hunks)arena_test.go(0 hunks)arena_timing_test.go(0 hunks)go.mod(1 hunks)handy.go(9 hunks)mergevalues.go(5 hunks)mergevalues_test.go(23 hunks)parser.go(15 hunks)parser_test.go(4 hunks)parser_timing_test.go(0 hunks)pool.go(0 hunks)pool_test.go(0 hunks)scanner.go(1 hunks)update.go(3 hunks)update_example_test.go(2 hunks)update_test.go(5 hunks)util.go(2 hunks)
💤 Files with no reviewable changes (6)
- pool_test.go
- pool.go
- parser_timing_test.go
- arena.go
- arena_test.go
- arena_timing_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
parser_test.go (2)
parser.go (1)
Parser(38-40)handy.go (2)
Parse(125-128)ParseWithArena(130-133)
update.go (1)
parser.go (4)
Object(511-514)Value(629-634)TypeObject(688-688)TypeArray(691-691)
mergevalues_test.go (2)
mergevalues.go (4)
MergeValues(16-111)ErrMergeDifferentTypes(11-11)ErrMergeDifferingArrayLengths(12-12)MergeValuesWithPath(113-127)handy.go (1)
MustParse(139-145)
handy.go (1)
parser.go (2)
Parser(38-40)Value(629-634)
mergevalues.go (1)
parser.go (2)
Value(629-634)TypeObject(688-688)
update_test.go (2)
parser.go (1)
Object(511-514)handy.go (1)
MustParse(139-145)
parser.go (1)
handy.go (2)
ParseWithArena(130-133)ParseBytesWithArena(155-158)
update_example_test.go (1)
handy.go (1)
MustParse(139-145)
🪛 GitHub Check: Linters
handy.go
[failure] 130-130:
undefined: arena (typecheck)
[failure] 155-155:
undefined: arena (typecheck)
mergevalues.go
[failure] 16-16:
undefined: arena (typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Success
🔇 Additional comments (15)
scanner.go (1)
64-64: LGTM! Consistent with arena-based parsing pattern.The change correctly passes
nilas the arena parameter toparseValue, indicating default heap allocation behavior. This aligns with the broader refactor to support arena-based memory management while maintaining backward compatibility.parser_test.go (2)
100-100: LGTM! Test correctly uses nil arena for default behavior.The test properly passes
nilas the arena parameter tounescapeStringBestEffort, maintaining the test's focus on string unescaping logic while accommodating the new arena-aware signature.
1292-1318: Excellent benchmark coverage for arena performance comparison.The two benchmarks effectively measure:
BenchmarkParse: Baseline performance with standard heap allocationBenchmarkParseArena: Arena-based allocation with proper arena reset between iterationsThe arena benchmark correctly:
- Pre-allocates a 2MB arena buffer (
WithMinBufferSize(1024 * 1024 * 2))- Resets the arena between iterations (
a.Reset())- Measures the same workload for fair comparison
This provides clear performance data to justify the arena integration.
util.go (3)
35-35: LGTM! Correctly passes nil for parent arena.The call
array.SetArrayItem(nil, len(items), value)properly passesnilas the arena/parent parameter, indicating default allocation behavior when appending to an array.
44-44: LGTM! Consistent arena parameter usage.The
Set(nil, path[i], child)call correctly passesnilas the arena parameter when creating intermediate path nodes in theSetValuefunction.
48-48: LGTM! Final set operation uses nil arena.The final
Set(nil, path[len(path)-1], value)call correctly completes the path-based value assignment with default allocation behavior.update_example_test.go (1)
53-64: LGTM! Example correctly demonstrates arena-based API usage.The example effectively shows:
- Creating a
MonotonicArenainstance (line 53)- Passing the arena to all
Setoperations (lines 55, 57, 61, 63)- Using arena for both direct and chained
Get(...).Set(...)callsThis provides clear guidance for users adopting the arena-based API while maintaining expected output.
update_test.go (2)
12-56: LGTM! Test correctly uses arena for object mutations.The test properly:
- Creates an arena instance (line 12)
- Passes the arena to
Setoperations (lines 39, 56)- Handles nil object/value cases gracefully (lines 54-56)
The arena usage is consistent and maintains test correctness.
61-107: LGTM! Comprehensive test coverage for arena-based value operations.The test effectively validates:
- Arena-based
Setoperations on values (line 87)- Array item updates with arena (lines 91, 94)
- Nil value handling (lines 103-106)
The use of a separate arena instance (
ar) and consistent parameter passing demonstrates proper API usage.mergevalues_test.go (1)
14-256: LGTM! Comprehensive test coverage maintained with arena-aware API.All tests consistently pass
nilas the arena parameter toMergeValuesandMergeValuesWithPath, demonstrating backward-compatible default behavior. The test cases cover:
- Nil value handling (lines 14, 25)
- Type mismatches (lines 36, 44, 53)
- Various data types (numbers, floats, arrays, objects, strings)
- Array length validation (line 143)
- Boolean merging (lines 159, 168)
- Null handling (lines 177, 186, 192)
- Nested structures (lines 198, 207)
- Path-based merging (lines 234, 240, 249)
The consistent nil arena usage indicates the tests focus on merge logic rather than arena allocation behavior.
mergevalues.go (2)
16-126: LGTM! Arena parameter correctly threaded through merge operations.The refactor properly:
- Adds
arena.Arenaparameter to both exported functions (lines 16, 113)- Passes arena to
unescapeKeys(lines 38-39)- Uses arena in
Setoperations (lines 45, 53, 122, 125)- Threads arena through recursive
MergeValuescalls (lines 48, 70, 115, 126)The arena integration is consistent and maintains the existing merge logic while enabling arena-based memory management.
6-7: Static analysis error resolved:arenaimport is correct andgo build ./...succeeds.go.mod (2)
7-7: Verify go-arena pseudo-version dependency
The module pathgithub.com/wundergraph/go-arena v0.0.0-20251008210416-55cb97e6f68fisn’t found publicly. Confirm this is the correct, actively maintained repo (or a private module) and, if possible, switch to a tagged release to ensure API stability, semantic versioning and reproducibility.
3-3: No changes needed for Go version requirement
Go 1.25 is available (go1.25.1), so the go.mod bump is valid.parser.go (1)
52-55: Arena-plumbed parse path looks good.ParseWithArena/ParseBytesWithArena and internal parse(a, ...) correctly propagate arena through parseValue/parseArray/parseObject.
Also applies to: 66-68, 70-82
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
23-27: Update GitHub Actions to latest versions.Static analysis indicates that the action versions are outdated:
actions/setup-go@v4should be updated to@v5golangci/golangci-lint-action@v3should be updated to@v4Apply these diffs to update the actions:
- name: Check out code into the Go module directory uses: actions/checkout@v3 - - name: Set up Go ${{ matrix.go }} - uses: actions/setup-go@v4 + - name: Set up Go ${{ matrix.go }} + uses: actions/setup-go@v5 with: go-version: ^${{ matrix.go }} id: go- name: Run linters - uses: golangci/golangci-lint-action@v3 + uses: golangci/golangci-lint-action@v4 with: version: v2.5.0 args: --timeout=3mAlso applies to: 45-47
🧹 Nitpick comments (1)
parser.go (1)
206-211: Consider larger initial array capacity.The array parsing allocates the initial slice with capacity 1 (
arena.AllocateSlice[*Value](a, 1, 1)), which meansarena.SliceAppendwill need to reallocate and copy when the second element is added. For better performance, consider starting with a larger capacity (e.g., 4 or 8) to reduce allocations for small-to-medium arrays.Apply this diff to use a larger initial capacity:
if arr.a == nil { - arr.a = arena.AllocateSlice[*Value](a, 1, 1) + arr.a = arena.AllocateSlice[*Value](a, 1, 4) arr.a[0] = v } else { arr.a = arena.SliceAppend(a, arr.a, v) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(2 hunks)parser.go(16 hunks)parser_timing_test.go(0 hunks)validate.go(1 hunks)values.go(1 hunks)
💤 Files with no reviewable changes (1)
- parser_timing_test.go
✅ Files skipped from review due to trivial changes (1)
- validate.go
🧰 Additional context used
🧬 Code graph analysis (2)
parser.go (1)
handy.go (2)
ParseWithArena(130-133)ParseBytesWithArena(155-158)
values.go (1)
parser.go (7)
Value(629-634)TypeString(694-694)TypeNumber(697-697)TypeTrue(700-700)TypeFalse(703-703)TypeObject(688-688)TypeArray(691-691)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
41-41: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: the runner of "golangci/golangci-lint-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
values.go (3)
23-35: LGTM!The numeric value constructors correctly allocate the
Valuein the arena and format the numbers as strings. Whilefmt.Sprintfallocates the formatted string on the heap rather than in the arena, this is acceptable because:
- The string representations are typically small
- The
Valuestruct itself is arena-allocated, which is the primary benefit- String storage optimization would add significant complexity
56-66: LGTM!The
ObjectValueandArrayValueconstructors correctly allocate arena-basedValueinstances with appropriate type tags. Theoandafields are left uninitialized, which is correct because they are lazily initialized when elements are added (as seen inparseArrayandparseObjectin parser.go).
9-54: LGTM!The string, number, and boolean value constructors are correctly implemented:
- Arena-based allocation using
arena.Allocate[Value](a)- Appropriate type tags set via
v.t- String fields properly assigned where needed
StringValueBytescorrectly usesb2sfor byte-to-string conversionparser.go (4)
47-82: LGTM!The parser methods correctly implement both backward-compatible (non-arena) and new arena-enabled parsing paths:
Parse()andParseBytes()maintain backward compatibility by passingnilarenaParseWithArena()andParseBytesWithArena()provide new arena-enabled entry points- All methods properly delegate to the internal
parse()function
512-512: LGTM with performance note.The change from
kvs []kvtokvs []*kvenables proper arena-based allocation of key-value pairs. While this adds a level of indirection (which can impact cache locality), it's necessary for arena semantics where each allocation is tracked by the arena for bulk deallocation.Also applies to: 553-559
349-432: LGTM!The
unescapeStringBestEffortfunction has been correctly converted to arena-based allocation:
- Reasonable capacity estimation (
len(s) + 4) to minimize reallocations- Consistent use of
arena.SliceAppendfor all byte slice operations- Preserves the original logic while integrating arena semantics
597-597: Verify nil arena behavior in unescapeKeys: ensure passing nil to o.unescapeKeys (in Object.Get/Visit) won’t panic—confirm go-arena falls back to heap allocations when Arena is nil or add appropriate nil checks and tests..github/workflows/ci.yml (1)
15-15: Configuration uses valid Go version. Go 1.25.0 was released on August 12 2025, with patch 1.25.2 on October 7 2025—no updates required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
47-49: golangci-lint binary version is invalid (v2.x does not exist).The action will fail downloading v2.5.0. Drop the override or pin to a real v1.x binary.
Apply one of these diffs:
- - name: golangci-lint - uses: golangci/golangci-lint-action@v8 - with: - version: v2.5.0 + - name: golangci-lint + uses: golangci/golangci-lint-action@v8Or pin to a valid v1.x (example):
- - name: golangci-lint - uses: golangci/golangci-lint-action@v8 - with: - version: v2.5.0 + - name: golangci-lint + uses: golangci/golangci-lint-action@v8 + with: + version: v1.61.0If you prefer pinning to latest, verify the current v1.x:
What is the latest golangci-lint release tag?
🧹 Nitpick comments (1)
update_test.go (1)
12-12: Consider releasing arenas in tests (if API provides it).If go-arena exposes Release/Reset/Close, add a defer to avoid lingering allocations and to mirror production usage.
Please confirm the correct teardown API name for github.com/wundergraph/go-arena and whether it's recommended in tests.
Also applies to: 61-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)update.go(3 hunks)update_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
update.go (1)
parser.go (4)
Object(511-514)Value(629-634)TypeObject(688-688)TypeArray(691-691)
update_test.go (2)
parser.go (2)
Parser(38-40)Object(511-514)handy.go (2)
MustParse(139-145)Parse(125-128)
🔇 Additional comments (2)
update_test.go (1)
122-151: Confirm nil-arena guarantee; test name assumes specific allocator semantics.The test asserts that calling code paths using unescapeKeys(nil) is safe. Please confirm that github.com/wundergraph/go-arena explicitly supports nil arenas (i.e., operations like internal slice growth fall back to heap) so this remains stable across versions. If not guaranteed, guard nil internally.
Based on learnings
update.go (1)
26-27: Recheck nil-arena assumption; avoid relying on undocumented fallback.Comment claims nil falls back to heap. Please confirm github.com/wundergraph/go-arena guarantees this across APIs used by unescapeKeys; otherwise guard nil and use heap paths explicitly.
Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
parser_timing_test.go (2)
101-131: Avoid sharing a singleParseracross workersLine 102 declares
benchPoolonce, but Lines 118-129 callbenchPool.Parsefrom multiple goroutines spawned byRunParallel.Parserretains mutable state (b []byte), so concurrent reuse corrupts the buffer and introduces data races. Give each worker its ownParser.-func benchmarkObjectGet(b *testing.B, itemsCount, lookupsCount int) { - var benchPool Parser +func benchmarkObjectGet(b *testing.B, itemsCount, lookupsCount int) { @@ - b.RunParallel(func(pb *testing.PB) { - for pb.Next() { - v, err := benchPool.Parse(s) + b.RunParallel(func(pb *testing.PB) { + var parser Parser + for pb.Next() { + v, err := parser.Parse(s)
233-268: IsolateParserstate inside each parallel workerLine 234’s
benchPoolis shared, yet Lines 241-266 parse concurrently viaRunParallel. The sharedParser’s mutable buffer makes this a data race. Allocate a separateParserper worker to keep parsing state isolated.-func benchmarkFastJSONParseGet(b *testing.B, s string) { - var benchPool Parser +func benchmarkFastJSONParseGet(b *testing.B, s string) { @@ - b.RunParallel(func(pb *testing.PB) { - var n int - for pb.Next() { - v, err := benchPool.Parse(s) + b.RunParallel(func(pb *testing.PB) { + var ( + parser Parser + n int + ) + for pb.Next() { + v, err := parser.Parse(s)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser_timing_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
parser_timing_test.go (2)
parser.go (3)
Parser(38-40)Type(681-681)TypeObject(688-688)handy.go (1)
Parse(125-128)
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores