Skip to content

Commit bb677e5

Browse files
olwangclaude
andcommitted
test(reg-vm): fix stale scalar-param test + add deterministic elision regression guard
Two fixes, both test-only (+ a cfg(test) helper): 1. FIX RED LIB TEST introduced by the elision default-flip (9d38eba): `lowering_omits_deep_copy_for_scalar_params_only` asserted a read heap param retains a raw `RegInstr::DeepCopy`, but with elision default-ON the lowerer neutralizes it to `DeepCopyElided`, so the assertion failed. (It was masked on the warm main tree by a stale test binary — the known docker cargo clock-skew; a fresh worktree build is red.) The test's intent is scalar-vs-heap (heap params carry a copy-isolation marker, scalars don't), so it now accepts either `DeepCopy | DeepCopyElided` — flag-agnostic, intent preserved. 2. ADD regression guard for the elision win (deterministic, timing-free): `deepcopy_elision_fires_for_read_only_heap_param` lowers a read-only-heap-param program and asserts the prologue `DeepCopy` is neutralized to `DeepCopyElided` under the default; under RSS_VM_ELIDE_DEEPCOPY=0 it asserts the eager copy is retained. If a future change stops the elision from firing, `DeepCopyElided` disappears and a raw `DeepCopy` returns → this fails. Chosen over a perf-gate case because jit_perf_gate measures only the NATIVE metric; this win is interpreter-side, so timing there wouldn't protect it. Adds `elide_deepcopy_enabled_for_test()` (cfg(test)) so the test tracks the real gate. Verified in a FRESH worktree (avoids the stale-binary trap): lib 277/0, main 41/0, runtime 455/0, differential 33/0, cost-model 1/0, soak 7/0, static 627/0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9d38eba commit bb677e5

2 files changed

Lines changed: 112 additions & 6 deletions

File tree

crates/rsscript/src/reg_vm/model.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1852,6 +1852,14 @@ fn elide_deepcopy_enabled() -> bool {
18521852
})
18531853
}
18541854

1855+
/// Test-only view of the process-once elision gate, so the elision regression guard
1856+
/// (`deepcopy_elision_fires_for_read_only_heap_param`) can branch on the exact verdict
1857+
/// the lowerer used rather than re-reading the env (and matching the memoized value).
1858+
#[cfg(test)]
1859+
pub(crate) fn elide_deepcopy_enabled_for_test() -> bool {
1860+
elide_deepcopy_enabled()
1861+
}
1862+
18551863
/// The register mutated IN PLACE by an in-scope container mutator, if any — a NON-`native-jit`
18561864
/// mirror of `native::passes::native_heap_mutation_receiver` (which is `cfg`-gated). Mutating
18571865
/// one of these on a tainted receiver would write through the shared `Rc` the interpreter would

crates/rsscript/src/reg_vm/tests.rs

Lines changed: 104 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -546,12 +546,19 @@ fn main() -> Unit {
546546

547547
let list_len_id = executable.unit.function_ids["list_len"];
548548
let list_len = executable.unit.functions[list_len_id].as_ref();
549-
assert!(
550-
list_len
551-
.code
552-
.iter()
553-
.any(|instr| matches!(instr, RegInstr::DeepCopy { .. })),
554-
"read heap/value params must retain copy isolation: {:#?}",
549+
// A `read` heap/value param always carries a prologue copy-isolation MARKER in
550+
// its slot: an eager `DeepCopy` when elision is off, or a neutralized
551+
// `DeepCopyElided` when the compile-time elision pass (default ON) proves the
552+
// copy redundant. Either way the slot is present (scalars emit neither) — this
553+
// is the scalar-vs-heap distinction the test guards, independent of the elision
554+
// flag. The elision-specific assertion lives in
555+
// `deepcopy_elision_fires_for_read_only_heap_param`.
556+
assert!(
557+
list_len.code.iter().any(|instr| matches!(
558+
instr,
559+
RegInstr::DeepCopy { .. } | RegInstr::DeepCopyElided { .. }
560+
)),
561+
"read heap/value params must carry a copy-isolation marker: {:#?}",
555562
list_len.code,
556563
);
557564
let output = executable
@@ -560,6 +567,97 @@ fn main() -> Unit {
560567
assert_eq!(output.stdout, "43\n");
561568
}
562569

570+
/// Regression guard for the compile-time `DeepCopy`-elision perf win (the ~16x
571+
/// speedup on `benchmarks/vm-jit/kernels/deepcopy_read_param.rss`). Mirrors that
572+
/// kernel's hot shape: a NON-`mut` heap param (`g: read Bag`) used only through
573+
/// read paths (`GetField` on `g`, then `List.len`/`List.get`), called from another
574+
/// function. Under the DEFAULT (`RSS_VM_ELIDE_DEEPCOPY` unset ⇒ elision ON) the
575+
/// lowerer must PROVE the prologue `DeepCopy` of `g` redundant and neutralize it to
576+
/// `RegInstr::DeepCopyElided` — the marker that turns the per-call deep copy into a
577+
/// cheap `Rc` share. If a future change re-introduces the eager copy (elision stops
578+
/// firing) `DeepCopyElided` disappears and a raw `DeepCopy` returns, failing this
579+
/// test. Run with `RSS_VM_ELIDE_DEEPCOPY=0` to see the guarded regression: the
580+
/// elided marker is gone and the eager `DeepCopy` is back.
581+
#[test]
582+
fn deepcopy_elision_fires_for_read_only_heap_param() {
583+
let source = r#"
584+
features: local
585+
586+
struct Bag {
587+
a: List<Int>,
588+
b: List<Int>,
589+
}
590+
591+
fn sum_reads(g: read Bag, i: Int) -> Int {
592+
let na = List.len<Int>(list: read g.a)
593+
let nb = List.len<Int>(list: read g.b)
594+
let va = List.get<Int>(list: read g.a, index: i % na)
595+
let vb = List.get<Int>(list: read g.b, index: i % nb)
596+
return va + vb
597+
}
598+
599+
fn caller(g: read Bag) -> Int {
600+
return sum_reads(g: read g, i: 0)
601+
}
602+
603+
fn main() -> Unit {
604+
local a = List.new<Int>()
605+
List.push<Int>(list: mut a, value: read 7)
606+
local b = List.new<Int>()
607+
List.push<Int>(list: mut b, value: read 5)
608+
let bag = Bag(a: take a, b: take b)
609+
Log.write(message: read String.from_int(value: caller(g: read bag)))
610+
return Unit
611+
}
612+
"#;
613+
let executable = reg_vm_compile_source("deepcopy-elision.rss", source)
614+
.expect("lowering should succeed");
615+
616+
let sum_reads_id = executable.unit.function_ids["sum_reads"];
617+
let sum_reads = executable.unit.functions[sum_reads_id].as_ref();
618+
let elided = sum_reads
619+
.code
620+
.iter()
621+
.filter(|instr| matches!(instr, RegInstr::DeepCopyElided { .. }))
622+
.count();
623+
let eager = sum_reads
624+
.code
625+
.iter()
626+
.filter(|instr| matches!(instr, RegInstr::DeepCopy { .. }))
627+
.count();
628+
// Under the default the read-only heap param's copy is elided; the elision is
629+
// gated (`elide_deepcopy_enabled`) so an explicit `RSS_VM_ELIDE_DEEPCOPY=0`
630+
// process would instead leave the eager `DeepCopy` — which is exactly the
631+
// regression this guard is meant to catch.
632+
if crate::reg_vm::model::elide_deepcopy_enabled_for_test() {
633+
assert!(
634+
elided >= 1,
635+
"elision ON: read-only heap param DeepCopy should be neutralized to \
636+
DeepCopyElided, got {elided} elided / {eager} eager: {:#?}",
637+
sum_reads.code,
638+
);
639+
assert_eq!(
640+
eager, 0,
641+
"elision ON: no eager DeepCopy should remain for the read-only heap \
642+
param: {:#?}",
643+
sum_reads.code,
644+
);
645+
} else {
646+
assert!(
647+
eager >= 1 && elided == 0,
648+
"elision OFF: eager DeepCopy must be retained (perf-win regression path): \
649+
got {elided} elided / {eager} eager: {:#?}",
650+
sum_reads.code,
651+
);
652+
}
653+
654+
// Elision (or its absence) must never change observable behavior.
655+
let output = executable
656+
.eval_main_with_args(Vec::<String>::new())
657+
.expect("program should still run");
658+
assert_eq!(output.stdout, "12\n");
659+
}
660+
563661
#[test]
564662
fn jit_runs_scalar_self_recursion_on_flat_executor() {
565663
let source = r#"

0 commit comments

Comments
 (0)