Phase 2b.3: switch Value::Text to Arc<String> with RC=1 in-place concat#276
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Phase 2b.3 of the RC-aware mutation rollout. Mirrors the Map switch (PR #261) and List switch (PR #273): cloning a Value::Text is now a refcount bump rather than a full byte copy, and the existing self-rebind concat peephole gains a Text branch in the next commit so the string-accumulator pattern folds from O(n^2) to O(n) amortised. This commit is the mechanical rewrite: every Value::Text(String) construction becomes Value::Text(Arc::new(...)) across the interpreter, VM, Cranelift JIT, json bridge, http/mcp tool providers, and the CLI arg-parser. Every Value::Text(s) read where the caller needs an owned String (MapKey::Text, serde_json::Value::String, parts: Vec<String>, etc.) becomes (**s).clone(). No behavioural changes. The peephole and its tests come next.
Extends the existing eval_self_rebind_concat fast path (added for Lists in PR #273) to also cover the Text/Text shape. When the prev binding and the rhs are both Value::Text, the peephole takes prev out of env to drop the Arc refcount to 1, then uses Arc::make_mut + push_str to mutate the inner String in place. The classic string-accumulator loop s=""; @i 0..n { s=+s "x" } drops from O(n^2) to O(n) amortised on the tree engine. Mirror of the VM OP_ADD_SS rebind-shape guard (PR #260) and the Cranelift jit_concat non-rebind split (PR #250). The alias guard inherited from match_self_rebind_concat (which calls expr_refers_to to reject `s = s + s` self-aliasing) carries over unchanged; the new Text branch leans on the same invariant the List branch relies on. Non-rebind shapes (`b = +a c`) still go through the general apply_binop path so the caller's `a` is preserved.
Adds tests/regression_string_accumulator_tree.rs with 10 cases
exercising the Phase 2b.3 Text peephole on the tree engine:
- literal / variable / Call-returning-Text rhs
- foreach accumulator (the hot shape this PR targets)
- non-rebind (`b = +a c`) preserves the caller's `a`
- self-alias (`s = +s s`) doubles correctly because the peephole
bails out via expr_refers_to
- numeric `n = +n 1` still falls through apply_binop unchanged
- 5k-iteration scale check under 5s, pinning the O(n) curve
Plus examples/string-accumulator-tree.ilo so the engine harness
runs the same shapes across tree, VM, and Cranelift, and so agents
encountering the pattern get an in-context learning example.
Rebase fallout. PR #275 (Ok-wrapper stdout fix) added a second NanVal->Value conversion via to_value_with_program with the HeapObj::Str(s) => Value::Text(s.clone()) shape that worked under the old Value::Text(String) representation. Phase 2b.3 makes Value::Text hold Arc<String>, so the bare s.clone() (which yields String) no longer satisfies the variant. Wrap it in Arc::new the same way the existing to_value site at line 3881 does.
0974b0d to
4fe5ea9
Compare
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third (and last) step of the tree-walker RC-aware mutation rollout. Mirror of Phase 2b.1 (Map, PR #261) and Phase 2b.2 (List, PR #273):
Value::Text(String)becomesValue::Text(Arc<String>)so cloning a string is a refcount bump rather than a full byte copy.eval_self_rebind_concat(added for Lists in interpreter: RC-aware Value::List with self-rebind append peephole #273) gains a Text branch. When prev and rhs are bothValue::Text,Arc::make_mutruns on the refcount=1 Arc andpush_strmutates the inner String in place.The classic string-accumulator loop folds from O(n^2) to O(n) amortised on the tree engine. 5k iterations of
s=+s "x"go from 10+ seconds to milliseconds.Repro
String.Arc::make_mut+push_str.The alias guard inherited from
match_self_rebind_concat(which callsexpr_refers_toto rejects = s + s) carries over unchanged. Non-rebind shapes (b = +a c) still go throughapply_binopso the caller'sais preserved, same as the VMOP_ADD_SSsplit landed in #260 and the Craneliftjit_concatsplit landed in #250.What's in the diff
interpreter: switch Value::Text to Arc<String> with RC=1 in-place concat(8a05a30) - mechanical rewrite. EveryValue::Text(String)construction becomesValue::Text(Arc::new(...))across the interpreter, VM, Cranelift JIT, json bridge, http/mcp tool providers, and the CLI arg-parser. EveryValue::Text(s)read that needs an ownedString(MapKey::Text, serde_json::Value::String, parts: Vec, etc.) becomes(**s).clone(). No behavioural change on its own.interpreter: eval_stmt peephole for self-rebind text concat(daab1c1) - adds the(Value::Text, Value::Text)branch toeval_self_rebind_concatand updates thematch_self_rebind_concatdocstring.test + example: tree-walker string accumulator regression coverage(0974b0d) - 10 regression tests covering literal / variable / Call-returning-Text rhs / foreach / non-rebind / self-alias / numeric fallthrough / 5k scale, plusexamples/string-accumulator-tree.iloso the engine harness runs the same shapes across tree, VM, and Cranelift.Test plan
cargo build --release --features craneliftcleancargo test --release --features cranelift- all 112 suites greencargo fmt --checkcleancargo clippy --release --features cranelift --all-targets -- -D warningscleanexamples/string-accumulator-tree.iloacross tree, VM, CraneliftFollow-ups
None. This closes Phase 2b. Map (#261), List (#273), and Text (this PR) all use the same Arc + self-rebind peephole shape on the tree engine, matching the VM
OP_*rebind splits and the Craneliftjit_*rebind splits that landed earlier.No README / SPEC updates: this is a runtime-internal perf fix with no user-observable semantic change beyond the formerly O(n^2) accumulator going O(n).