perf(codegen): append strings through variable cells

changelog.d/8454-string-accumulators.md

@@ -0,0 +1,6 @@
+### Performance
+
+- Keep string accumulators stored in async/closure variable boxes, captures,
+  and module globals on Perry's amortized append path. Ordinary reads and
+  local-to-local assignments still demote extracted aliases before later
+  in-place growth.

crates/perry-codegen/src/codegen/declared_string_add_tests.rs

@@ -106,8 +106,7 @@ fn ir(params: Vec<Param>, body: Expr) -> String {
 }
 
 fn function_ir(module: Module) -> String {
-    let ir =
-        String::from_utf8(compile_module(&module, ir_opts()).unwrap()).expect("LLVM IR is UTF-8");
+    let ir = module_ir(module);
     // An ordinary typed parameter may now produce a public guard wrapper plus
     // proof-bearing and generic clones (#8079). This suite's subject remains
     // the annotation-distrusting body, so inspect the generic clone when one
@@ -134,6 +133,10 @@ fn function_ir(module: Module) -> String {
     ir[start..end].to_string()
 }
 
+fn module_ir(module: Module) -> String {
+    String::from_utf8(compile_module(&module, ir_opts()).unwrap()).expect("LLVM IR is UTF-8")
+}
+
 fn add(left: Expr, right: Expr) -> Expr {
     Expr::Binary {
         op: BinaryOp::Add,
@@ -394,6 +397,208 @@ fn a_self_append_chain_keeps_an_opaque_numeric_head_pair_intact() {
     );
 }
 
+#[test]
+fn a_module_global_self_append_uses_the_amortized_path_and_demotes_extractions() {
+    const GLOBAL: u32 = 10;
+    let value = add(
+        add(Expr::LocalGet(GLOBAL), Expr::String("[".to_string())),
+        Expr::String("]".to_string()),
+    );
+    let mut module = module_with(probe_fn_with_body(
+        Vec::new(),
+        vec![
+            Stmt::While {
+                condition: Expr::Bool(false),
+                body: vec![Stmt::Expr(Expr::LocalSet(GLOBAL, Box::new(value)))],
+            },
+            Stmt::Return(Some(Expr::LocalGet(GLOBAL))),
+        ],
+    ));
+    module.init.push(Stmt::Let {
+        id: GLOBAL,
+        name: "module_accumulator".to_string(),
+        ty: Type::String,
+        mutable: true,
+        init: Some(Expr::String(String::new())),
+    });
+    let ir = function_ir(module);
+
+    assert!(
+        ir.contains("call i64 @js_string_append("),
+        "a module root is binding storage and can retain the unique string owner:\n{ir}"
+    );
+    assert!(
+        ir.contains("call void @js_string_addref_if_heap_string("),
+        "returning the global extracts an alias and must demote it first:\n{ir}"
+    );
+}
+
+#[test]
+fn a_boxed_local_self_append_uses_the_amortized_path() {
+    const ACC: u32 = 10;
+    const READER: u32 = 11;
+    let value = add(Expr::LocalGet(ACC), Expr::String("long-part".to_string()));
+    let module = module_with(probe_fn_with_body(
+        Vec::new(),
+        vec![
+            Stmt::Let {
+                id: ACC,
+                name: "accumulator".to_string(),
+                ty: Type::String,
+                mutable: true,
+                init: Some(Expr::String(String::new())),
+            },
+            // Capturing a local that is also mutated in this scope makes the
+            // source binding a shared variable box.
+            Stmt::Let {
+                id: READER,
+                name: "reader".to_string(),
+                ty: Type::Any,
+                mutable: false,
+                init: Some(Expr::Closure {
+                    func_id: 2,
+                    params: Vec::new(),
+                    return_type: Type::String,
+                    body: vec![Stmt::Return(Some(Expr::LocalGet(ACC)))],
+                    captures: vec![ACC],
+                    mutable_captures: vec![ACC],
+                    captures_this: false,
+                    captures_new_target: false,
+                    enclosing_class: None,
+                    is_arrow: true,
+                    is_async: false,
+                    is_generator: false,
+                    is_strict: true,
+                }),
+            },
+            Stmt::While {
+                condition: Expr::Bool(false),
+                body: vec![Stmt::Expr(Expr::LocalSet(ACC, Box::new(value)))],
+            },
+            Stmt::Return(Some(Expr::LocalGet(ACC))),
+        ],
+    ));
+    let ir = function_ir(module);
+
+    assert!(
+        ir.contains("call i64 @js_string_append("),
+        "a variable box must retain the accumulator owner across iterations:\n{ir}"
+    );
+    assert!(
+        ir.contains("call i64 @js_box_get_bits(") && ir.contains("call void @js_box_set_bits("),
+        "the append result must be read from and written back to the box:\n{ir}"
+    );
+}
+
+#[test]
+fn a_boxed_capture_self_append_uses_the_amortized_path() {
+    const ACC: u32 = 10;
+    const APPENDER: u32 = 11;
+    let module = module_with(probe_fn_with_body(
+        Vec::new(),
+        vec![
+            Stmt::Let {
+                id: ACC,
+                name: "accumulator".to_string(),
+                ty: Type::String,
+                mutable: true,
+                init: Some(Expr::String(String::new())),
+            },
+            Stmt::Let {
+                id: APPENDER,
+                name: "append".to_string(),
+                ty: Type::Any,
+                mutable: false,
+                init: Some(Expr::Closure {
+                    func_id: 2,
+                    params: Vec::new(),
+                    return_type: Type::String,
+                    body: vec![
+                        Stmt::While {
+                            condition: Expr::Bool(false),
+                            body: vec![Stmt::Expr(Expr::LocalSet(
+                                ACC,
+                                Box::new(add(
+                                    Expr::LocalGet(ACC),
+                                    Expr::String("long-part".to_string()),
+                                )),
+                            ))],
+                        },
+                        Stmt::Return(Some(Expr::LocalGet(ACC))),
+                    ],
+                    captures: vec![ACC],
+                    mutable_captures: vec![ACC],
+                    captures_this: false,
+                    captures_new_target: false,
+                    enclosing_class: None,
+                    is_arrow: true,
+                    is_async: false,
+                    is_generator: false,
+                    is_strict: true,
+                }),
+            },
+            Stmt::Return(Some(Expr::LocalGet(APPENDER))),
+        ],
+    ));
+    let ir = module_ir(module);
+
+    assert!(
+        ir.contains("call i64 @js_string_append("),
+        "a captured variable box must reach the append helper:\n{ir}"
+    );
+    assert!(
+        ir.contains("call i64 @js_closure_get_capture_bits(")
+            && ir.contains("call void @js_box_set_bits("),
+        "the captured owner must be dereferenced and written through its box:\n{ir}"
+    );
+}
+
+#[test]
+fn a_module_global_numeric_capable_head_pair_does_not_select_append() {
+    const GLOBAL: u32 = 10;
+    let value = add(
+        add(Expr::LocalGet(GLOBAL), Expr::Number(1.0)),
+        Expr::String("x".to_string()),
+    );
+    let mut module = module_with(probe_fn_with_body(
+        Vec::new(),
+        vec![Stmt::While {
+            condition: Expr::Bool(false),
+            body: vec![Stmt::Expr(Expr::LocalSet(GLOBAL, Box::new(value)))],
+        }],
+    ));
+    module.init.push(Stmt::Let {
+        id: GLOBAL,
+        name: "lying_accumulator".to_string(),
+        ty: Type::String,
+        mutable: true,
+        init: Some(Expr::Number(42.0)),
+    });
+    let ir = function_ir(module);
+
+    assert!(
+        !ir.contains("call i64 @js_string_append("),
+        "the newly eligible storage must not weaken the numeric-head guard:\n{ir}"
+    );
+}
+
+#[test]
+fn assigning_one_local_to_another_demotes_a_possible_string_alias() {
+    let module = module_with(probe_fn_with_body(
+        vec![str_param(), param(2, "snapshot", Type::String)],
+        vec![
+            Stmt::Expr(Expr::LocalSet(2, Box::new(Expr::LocalGet(1)))),
+            Stmt::Return(Some(Expr::Number(0.0))),
+        ],
+    ));
+    let ir = function_ir(module);
+
+    assert!(
+        ir.contains("call void @js_string_addref_if_heap_string("),
+        "assignment aliases need the same demote as declaration aliases:\n{ir}"
+    );
+}
+
 // ------------------------------------------------------- untouched tiers
 
 #[test]

crates/perry-codegen/src/expr/literals_vars.rs

@@ -8,7 +8,9 @@ use anyhow::Result;
 use perry_hir::types::Type as HirType;
 use perry_hir::{BinaryOp, Expr, UpdateOp};
 
-use crate::lower_string_concat::{flatten_string_add_chain, lower_string_self_append};
+use crate::lower_string_concat::{
+    can_lower_string_self_append, flatten_string_add_chain, lower_string_self_append,
+};
 use crate::nanbox::double_literal;
 use crate::native_value::MaterializationReason;
 use crate::type_analysis::{is_map_expr, is_set_expr, receiver_class_name};
@@ -21,6 +23,25 @@ use super::{
     lower_pod_local_reassignment, materialize_pod_local, nanbox_string_inline, FnCtx,
 };
 
+/// A box, closure cell, or module root is the storage for the source binding,
+/// not an alias of the string it currently owns. An ordinary read extracts a
+/// second copy of that value, so demote a heap string before it can outlive the
+/// cell and be silently changed by a later in-place append (#8432).
+///
+/// The append lowering reads these targets directly and therefore deliberately
+/// bypasses this rule. Limit the call to declared-string bindings: only those
+/// bindings can select in-place append, and erased annotations remain safe
+/// because the runtime helper checks the live tag.
+fn demote_extracted_string_binding(ctx: &mut FnCtx<'_>, id: u32, value: &str) {
+    let persistent_binding = ctx.closure_captures.contains_key(&id)
+        || (ctx.boxed_vars.contains(&id) && !ctx.module_globals.contains_key(&id))
+        || ctx.module_globals.contains_key(&id);
+    if persistent_binding && matches!(ctx.local_type_hint(&id), Some(HirType::String)) {
+        ctx.block()
+            .call_void("js_string_addref_if_heap_string", &[(DOUBLE, value)]);
+    }
+}
+
 /// #1380: method names addressable on a `Set` instance, used by the
 /// `typeof set.<name>` fold to report "function" (Set method values are
 /// not materialized as real function objects). Includes the ES2024
@@ -404,14 +425,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                         &[(I64, &closure_ptr), (I32, &idx_str)],
                     );
                     let bits = blk.call(I64, "js_box_get_bits", &[(I64, &box_ptr)]);
-                    return Ok(blk.bitcast_i64_to_double(&bits));
+                    let value = blk.bitcast_i64_to_double(&bits);
+                    demote_extracted_string_binding(ctx, *id, &value);
+                    return Ok(value);
                 }
                 let bits = ctx.block().call(
                     I64,
                     "js_closure_get_capture_bits",
                     &[(I64, &closure_ptr), (I32, &idx_str)],
                 );
-                return Ok(ctx.block().bitcast_i64_to_double(&bits));
+                let value = ctx.block().bitcast_i64_to_double(&bits);
+                demote_extracted_string_binding(ctx, *id, &value);
+                return Ok(value);
             }
             // Boxed local in enclosing function: load the slot (box
             // pointer), deref via js_box_get_bits.
@@ -434,7 +459,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                     let blk = ctx.block();
                     let box_ptr = blk.load(I64, &slot);
                     let bits = blk.call(I64, "js_box_get_bits", &[(I64, &box_ptr)]);
-                    return Ok(blk.bitcast_i64_to_double(&bits));
+                    let value = blk.bitcast_i64_to_double(&bits);
+                    demote_extracted_string_binding(ctx, *id, &value);
+                    return Ok(value);
                 }
             }
             // Repsel Phase 1: a canonical-i32 local's ONLY storage is the i32
@@ -458,10 +485,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                     };
                     return Ok(v);
                 }
-                Ok(ctx.block().load(DOUBLE, &slot))
+                let value = ctx.block().load(DOUBLE, &slot);
+                demote_extracted_string_binding(ctx, *id, &value);
+                Ok(value)
             } else if let Some(global_name) = ctx.module_globals.get(id).cloned() {
                 let g_ref = format!("@{}", global_name);
-                Ok(ctx.block().load(DOUBLE, &g_ref))
+                let value = ctx.block().load(DOUBLE, &g_ref);
+                demote_extracted_string_binding(ctx, *id, &value);
+                Ok(value)
             } else {
                 // Soft fallback: the HIR sometimes carries stale
                 // local references that don't correspond to any
@@ -498,26 +529,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             // `js_string_concat_chain` would copy the growing `x` prefix on
             // every loop iteration, turning the otherwise-amortized append
             // path back into O(n^2) work (#8394).
-            // The fast path requires a plain alloca slot in `ctx.locals` —
-            // module globals (use `@global` loads), closure captures (use
-            // `js_closure_{get,set}_capture_bits`), and boxed vars (use
-            // `js_box_set_bits` through a heap cell) all need different store
-            // mechanics, so they fall through to the regular `LocalSet`
-            // path below. Issue #319: without the `ctx.locals.contains_key`
-            // / closure_captures / boxed_vars guards, a closure-captured
-            // string-typed local that does `s = s + t` aborted codegen
-            // with `string self-append: local N not in scope` because the
-            // helper's `ctx.locals.get(id)` lookup whiffed.
+            // The append helper abstracts over plain slots, module roots,
+            // closure captures, and variable boxes. Ordinary reads from the
+            // latter three storage families demote a heap string to shared;
+            // this owner read deliberately bypasses that extraction rule so
+            // the binding can retain uniqueness across iterations (#8432).
             // #7841: the tag-dispatched helper validates the destination's
             // current value before choosing append versus ordinary JS `+`.
             // This is therefore a dispatch hint, not a binding proof; using
             // the stable-only query would disable the optimization for every
             // self-append because this `LocalSet` is itself a reassignment.
             if matches!(ctx.local_type_hint(id), Some(HirType::String))
-                && !ctx.module_globals.contains_key(id)
-                && !ctx.closure_captures.contains_key(id)
-                && !ctx.boxed_vars.contains(id)
-                && ctx.locals.contains_key(id)
+                && can_lower_string_self_append(ctx, *id)
             {
                 if let Expr::Binary {
                     op: BinaryOp::Add,
@@ -615,6 +638,16 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             }
 
             let v = lower_expr(ctx, value)?;
+            // `target = source` creates the same string-buffer alias as a
+            // `let target = source` initializer. The declaration path has
+            // demoted this shape since #5552, but assignment aliases were
+            // previously missed. Async lowering expresses mid-body snapshot
+            // variables as LocalSet, making that gap observable as the saved
+            // string growing in place with its boxed accumulator (#8432).
+            if matches!(value.as_ref(), Expr::LocalGet(source_id) if source_id != id) {
+                ctx.block()
+                    .call_void("js_string_addref_if_heap_string", &[(DOUBLE, &v)]);
+            }
             // Closure captures first (write through the runtime), then
             // locals, then module globals.
             if let Some(&capture_idx) = ctx.closure_captures.get(id) {