diff --git a/changelog.d/8583-count-store-safepoints.md b/changelog.d/8583-count-store-safepoints.md new file mode 100644 index 0000000000..56e36504ea --- /dev/null +++ b/changelog.d/8583-count-store-safepoints.md @@ -0,0 +1 @@ +fix(codegen): the RS4GC root-spill estimate (`count_safepoint_sites`, #8583) now counts property/index STORES (`PropertySet`, `PropertyUpdate`, `IndexSet`) as GC safepoints, extending the earlier literal-counting fix. A closed-shape object literal compiles to a constructor that is one long run of `this.field = v` stores, each lowering to a collecting `js_class_field_set_ic` / `js_set_property` call that RS4GC gives a statepoint; with none counted, the constructor's estimate was ~0, it was never spilled, and RS4GC grew one such `__AnonShape_*_constructor` from 34k to 2.28M instructions — overrunning the #8586 per-function budget and refusing the whole module. Reads (`PropertyGet`/`IndexGet`) are deliberately not counted: they frequently inline to a shape-cached load with no call, and counting them would over-spill read-heavy hot loops. diff --git a/crates/perry-codegen/src/collectors/safepoint_sites.rs b/crates/perry-codegen/src/collectors/safepoint_sites.rs index 4cb67640f9..853422f36a 100644 --- a/crates/perry-codegen/src/collectors/safepoint_sites.rs +++ b/crates/perry-codegen/src/collectors/safepoint_sites.rs @@ -71,6 +71,20 @@ fn is_safepoint(e: &Expr) -> bool { | Expr::ObjectAssign { .. } | Expr::Array(_) | Expr::ArraySpread(_) + // #8583 (`__AnonShape_*_constructor`): a property/index STORE lowers + // to an allocating, collecting runtime call (`js_class_field_set_ic` + // / `js_set_property` / the array-set helpers) that RS4GC gives a + // statepoint. A closed-shape object literal compiles to a constructor + // that is one long run of `this.field = v` stores (`PropertySet`); + // with none counted the estimate was ~0, the constructor was not + // spilled, and RS4GC grew it 34k -> 2.28M instructions, overrunning + // the #8586 budget. Count the stores (not the reads: + // `PropertyGet`/`IndexGet` frequently inline to a shape-cached load + // with no call, and counting them would over-spill read-heavy hot + // loops). `PropertyUpdate` (`x.f++`) is a read-modify-write store. + | Expr::PropertySet { .. } + | Expr::PropertyUpdate { .. } + | Expr::IndexSet { .. } ) } @@ -257,4 +271,40 @@ mod tests { // 1 outer + 10 inner = 11. assert_eq!(count_safepoint_sites(&[Stmt::Expr(Expr::Array(rows))]), 11); } + + #[test] + fn property_and_index_stores_are_safepoints() { + // #8583: `this.field = v` / `arr[i] = v` lower to a collecting runtime + // call and must count. A closed-shape object literal is a constructor of + // many `PropertySet` stores (the `__AnonShape_*_constructor` shape) — the + // pre-fix count saw none, so the constructor never spilled and RS4GC + // overran the #8586 budget. + let this = || Expr::LocalGet(0); + let set = |p: &str| Expr::PropertySet { + object: Box::new(this()), + property: p.to_string(), + value: Box::new(Expr::Number(1.0)), + }; + // Three field stores in the constructor body. + let body = vec![Stmt::Expr(set("a")), Stmt::Expr(set("b")), Stmt::Expr(set("c"))]; + assert_eq!(count_safepoint_sites(&body), 3); + + // An index store counts too; the value sub-expression still recurses + // (a call in the value is its own safepoint). + let idx_set = Expr::IndexSet { + object: Box::new(this()), + index: Box::new(Expr::Number(0.0)), + value: Box::new(call(vec![])), + }; + // 1 for the IndexSet + 1 for the call in `value`. + assert_eq!(count_safepoint_sites(&[Stmt::Expr(idx_set)]), 2); + + // A read (`PropertyGet`) is deliberately NOT a safepoint (it inlines). + let get = Expr::PropertyGet { + object: Box::new(this()), + property: "x".to_string(), + byte_offset: 0, + }; + assert_eq!(count_safepoint_sites(&[Stmt::Expr(get)]), 0); + } }