perf: avoid rebuilding untouched nodes during expression rewrites

[robert3005]

Defer cloning children until one of them actually changes

Signed-off-by: Robert Kruszewski robert@spiraldb.com

[codspeed-hq]

Merging this PR will degrade performance by 12.07%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 6 regressed benchmarks
✅ 1994 untouched benchmarks
⏩ 89 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	Simulation	compact[(4096, 90)]	1.6 µs	1.9 µs	-13.97%
❌	Simulation	search_index_in_range_chunked	5.1 ms	5.9 ms	-13.52%
❌	Simulation	compact[(16384, 90)]	1.8 µs	2 µs	-13.01%
❌	WallTime	words_gather_dispatch[1024]	8 ns	9 ns	-11.11%
❌	Simulation	compact_sliced[(4096, 90)]	1.7 µs	1.9 µs	-10.51%
❌	Simulation	slice_primitive_tight_loop[10000]	422.3 µs	470.5 µs	-10.25%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing rk/expr-rewrite-fewer-clones (1d30b69) with develop (b825c4f)}

Footnotes

1. 89 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[connortsui20]

This is a comment from codex:

This optimization assumes every BoundExpression::Scalar was created through try_new, but the public enum does not enforce that invariant. Should we make construction opaque before relying on it here, or retain the validation in with_children?

[robert3005]

there are bigger problems if someone does this... not sure we can effectively restrict it though

[connortsui20]

Nice, this is clever.

Seems fine to me with the exception of that optimization (see comment from codex), but IIUC that is an existing issue because the field is public and thus there are no invariants

[connortsui20]

oh whoops auto merge was on.

I think its fine @robert3005