Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog.d/8897-field-push-writeback-handle-bits.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
### Fixed

- The `this.f.push(v)` expansion (`field_push_local_bind`) wrote the field back through a JS `!==` guard that never fired: a growing append leaves the old head forwarding to the new one and equality sees through forwarding, so the field kept the stub and every later `this.f.length` / `this.f[i]` took the dynamic property path (#8897 — a 2.5× cold-phase regression in the wolf-ecs entity cycle). `Expr::ArrayPush` now carries the field to write back and codegen compares the receiver local's handle bits before and after the append, re-pointing the field — behind an inline plain-object header gate — when they differ.
4 changes: 3 additions & 1 deletion crates/perry-codegen-js/src/emit/exprs_more.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ impl JsEmitter {
}

// --- Array methods ---
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
let name = self.get_local_name(*array_id);
let _ = write!(self.output, "{}.push(", name);
self.emit_expr(value);
Expand Down
4 changes: 3 additions & 1 deletion crates/perry-codegen-wasm/src/emit/expr/arrays.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ impl<'a> FuncEmitCtx<'a> {
}
}

Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
self.emit_local_or_global_get(func, array_id);
self.emit_frame_begin(func, 2);
func.instruction(&Instruction::LocalSet(self.temp_local));
Expand Down
4 changes: 3 additions & 1 deletion crates/perry-codegen-wasm/src/emit/js_fallback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,9 @@ impl WasmModuleEmitter {
obj, k, val, val
)
}
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
let arr = locals
.get(array_id)
.cloned()
Expand Down
7 changes: 5 additions & 2 deletions crates/perry-codegen/src/collectors/all_pointer_arrays.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
//! would point the local at an array the declaration never covered, and the
//! elided stores would then be describing the wrong object.
//! 3. **Every store into it is a push of an allocation, and there is at least
//! one.** Every `Expr::ArrayPush { array_id: id }` pushes a fresh
//! one.** Every `Expr::ArrayPush { array_id: id, .. }` pushes a fresh
//! allocation; no `Expr::ArrayPushSpread`, no `Expr::IndexSet` whose object
//! is `LocalGet(id)` (an indexed store can jump past `length`, a different
//! claim than "append"), no other in-place array mutation.
Expand Down Expand Up @@ -168,7 +168,9 @@ pub(crate) fn collect_all_pointer_array_locals(
Expr::ArrayPop(array_id) | Expr::ArrayShift(array_id) => {
killed.insert(*array_id);
}
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
if crate::expr::expr_produces_fresh_heap_allocation(value) {
pushed.insert(*array_id);
} else {
Expand Down Expand Up @@ -258,6 +260,7 @@ mod tests {
Stmt::Expr(Expr::ArrayPush {
array_id: id,
value: Box::new(value),
field_writeback: None,
})
}

Expand Down
2 changes: 1 addition & 1 deletion crates/perry-codegen/src/collectors/escape_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ pub fn check_escapes_in_expr(
check_escapes_in_expr(init, candidates, classes, escaped);
}
}
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush { array_id, value, .. } => {
if candidates.contains_key(array_id) {
escaped.insert(*array_id);
}
Expand Down
5 changes: 4 additions & 1 deletion crates/perry-codegen/src/collectors/hir_facts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,9 @@ impl ArrayFactCollector {

fn collect_expr(&mut self, expr: &Expr) {
match expr {
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
let value_kind = if expr_is_i32_shaped(value) {
ArrayKindFact::PackedI32
} else if expr_is_numeric_shaped(value) {
Expand Down Expand Up @@ -2297,6 +2299,7 @@ mod tests {
Stmt::Expr(Expr::ArrayPush {
array_id: 2,
value: Box::new(Expr::Integer(4)),
field_writeback: None,
}),
],
&HashSet::new(),
Expand Down
121 changes: 76 additions & 45 deletions crates/perry-codegen/src/collectors/hot_callees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,26 +65,49 @@ const TINY_METHOD_MAX_STMTS: usize = 2;
const TINY_METHOD_ALLOC_SITE_BUDGET: u32 = 8;

/// The receiver-binding local `perry-transform`'s `field_push_local_bind`
/// pass introduces when it expands one `this.f.push(v)` statement into four
/// (`let __push_recv_old = this.f; let __push_recv = old; push; if (moved)
/// this.f = __push_recv`). For the tiny-method budget above that is still the
/// ONE statement the author wrote: the pass exists so the push takes the
/// pass introduces when it expands one `this.f.push(v)` statement into two
/// (`let __push_recv = this.f; push` — the write-back rides on the
/// `ArrayPush` node itself). For the tiny-method budget above that is still
/// the ONE statement the author wrote: the pass exists so the push takes the
/// inline append, and a command-buffer method that is exactly
/// `this.commands.push({ ... })` must not lose its allocation kernel to the
/// rewrite that made its push cheaper. Kept in sync by name with the pass
/// (`field_push_local_bind.rs`); the test below pins the shape.
const FIELD_PUSH_RECEIVER_OLD_NAME: &str = "__push_recv_old";
const FIELD_PUSH_RECEIVER_NAME: &str = "__push_recv";

/// Statement count for the tiny-method rule: each field-push expansion
/// counts as the single statement it came from.
/// counts as the single statement it came from. An expansion is the COMPLETE
/// shape the pass emits — the receiver `let` immediately followed by the
/// `ArrayPush` on that local carrying the same field as its write-back — so
/// an author's own local that happens to be named `__push_recv` does not
/// shrink the count.
fn tiny_method_stmt_count(body: &[Stmt]) -> usize {
let expansions = body
.iter()
.filter(
|stmt| matches!(stmt, Stmt::Let { name, .. } if name == FIELD_PUSH_RECEIVER_OLD_NAME),
)
.windows(2)
.filter(|pair| {
matches!(
pair,
[
Stmt::Let {
id,
name,
mutable: true,
init: Some(Expr::PropertyGet { object, property, .. }),
..
},
Stmt::Expr(Expr::ArrayPush {
array_id,
field_writeback: Some(field),
..
}),
] if name == FIELD_PUSH_RECEIVER_NAME
&& array_id == id
&& matches!(object.as_ref(), Expr::This)
&& field == property
)
})
.count();
body.len().saturating_sub(3 * expansions)
body.len().saturating_sub(expansions)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

/// Collect the set of `FuncId`s eligible for `inlinehint`: those with ≥1 direct
Expand Down Expand Up @@ -866,43 +889,24 @@ mod recursion_participant_tests {
}

/// `this.commands.push({ ... })` after `field_push_local_bind` expanded it.
fn expanded_field_push(old_id: u32, recv_id: u32) -> Vec<Stmt> {
fn expanded_field_push(recv_id: u32) -> Vec<Stmt> {
vec![
Stmt::Let {
id: old_id,
name: FIELD_PUSH_RECEIVER_OLD_NAME.to_string(),
id: recv_id,
name: FIELD_PUSH_RECEIVER_NAME.to_string(),
ty: Type::Any,
mutable: false,
mutable: true,
init: Some(Expr::PropertyGet {
object: Box::new(Expr::This),
property: "commands".to_string(),
byte_offset: 0,
}),
},
Stmt::Let {
id: recv_id,
name: "__push_recv".to_string(),
ty: Type::Any,
mutable: true,
init: Some(Expr::LocalGet(old_id)),
},
Stmt::Expr(Expr::ArrayPush {
array_id: recv_id,
value: Box::new(new_expr()),
field_writeback: Some("commands".to_string()),
}),
Stmt::If {
condition: Expr::Compare {
op: perry_hir::CompareOp::Ne,
left: Box::new(Expr::LocalGet(recv_id)),
right: Box::new(Expr::LocalGet(old_id)),
},
then_branch: vec![Stmt::Expr(Expr::PropertySet {
object: Box::new(Expr::This),
property: "commands".to_string(),
value: Box::new(Expr::LocalGet(recv_id)),
})],
else_branch: None,
},
]
}

Expand Down Expand Up @@ -944,32 +948,59 @@ mod recursion_participant_tests {
let mut module = Module::new("buffer.ts");
module
.classes
.push(class_with_method(func(11, expanded_field_push(100, 101))));
let mut plain = expanded_field_push(200, 201);
// Same four statements, but the first is an ordinary local: not an
// expansion, so the method is four statements long.
.push(class_with_method(func(11, expanded_field_push(101))));
// Two copies back to back: two authored statements, not one.
let mut two = expanded_field_push(201);
two.extend(expanded_field_push(202));
module.classes.push(class_with_method(func(12, two)));
// The same two statements, but the local is an ordinary one: not an
// expansion, so the method is two statements long.
let mut plain = expanded_field_push(301);
if let Stmt::Let { name, .. } = &mut plain[0] {
*name = "old".to_string();
}
module.classes.push(class_with_method(func(12, plain)));
module.classes.push(class_with_method(func(13, plain)));
// An author's own local named like the receiver, followed by an
// unrelated statement: no expansion, two statements.
let mut collision = expanded_field_push(401);
collision[1] = Stmt::Expr(new_expr());
module.classes.push(class_with_method(func(14, collision)));
// The receiver `let` followed by a push WITHOUT a write-back target
// (an ordinary local push that merely shares the name): two.
let mut no_target = expanded_field_push(501);
if let Stmt::Expr(Expr::ArrayPush {
field_writeback, ..
}) = &mut no_target[1]
{
*field_writeback = None;
}
module.classes.push(class_with_method(func(15, no_target)));

assert_eq!(
tiny_method_stmt_count(&module.classes[0].methods[0].body),
1
);
assert_eq!(
tiny_method_stmt_count(&module.classes[1].methods[0].body),
4
2
);
assert_eq!(
tiny_method_stmt_count(&module.classes[2].methods[0].body),
2
);
assert_eq!(
tiny_method_stmt_count(&module.classes[3].methods[0].body),
2
);
assert_eq!(
tiny_method_stmt_count(&module.classes[4].methods[0].body),
2
);
let hot = collect_alloc_hot_functions(&module);
assert!(
hot.contains(&11),
"the expanded field push is still a tiny kernel: {hot:?}"
);
assert!(
!hot.contains(&12),
"four unrelated statements are not: {hot:?}"
);
}

#[test]
Expand Down
4 changes: 3 additions & 1 deletion crates/perry-codegen/src/collectors/mutation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ pub fn expr_has_mutation(e: &perry_hir::Expr, id: u32) -> bool {
Expr::Object(props) => props.iter().any(|(_, v)| expr_has_mutation(v, id)),
Expr::Closure { body, .. } => has_any_mutation(body, id),
Expr::Sequence(es) => es.iter().any(|e| expr_has_mutation(e, id)),
Expr::ArrayPush { array_id, value } => *array_id == id || expr_has_mutation(value, id),
Expr::ArrayPush {
array_id, value, ..
} => *array_id == id || expr_has_mutation(value, id),
Expr::ArraySplice {
array_id,
start,
Expand Down
5 changes: 4 additions & 1 deletion crates/perry-codegen/src/collectors/ptr_numarray.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,9 @@ impl<'a> UseWalk<'a> {
// Numeric push keeps every invariant (canonical store through the
// Phase 4a.1 tiers; growth writes the live head back to the
// slot). A possibly-non-numeric push value disqualifies.
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
if self.is_candidate(*array_id) && !self.value_is_numeric(*array_id, value) {
self.disq(*array_id);
}
Expand Down Expand Up @@ -955,6 +957,7 @@ mod tests {
Expr::ArrayPush {
array_id: id,
value: Box::new(value),
field_writeback: None,
}
}

Expand Down
4 changes: 3 additions & 1 deletion crates/perry-codegen/src/collectors/ptr_shape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,9 @@ impl<'a> UseWalk<'a> {
// are bounded by `collectors/ptr_shape_elements.rs` exactly as
// rule 2 bounds an object local's, so no alias escapes the region.
// Any other array, any other value shape, keeps today's escape.
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
self.disq(*array_id, report::ESC_CONTAINER_MUTATOR);
// #7770: record the provenance argument list for the
// group-wide numeric proof. Only pushes into a PROVEN array
Expand Down
6 changes: 4 additions & 2 deletions crates/perry-codegen/src/collectors/ptr_shape_elements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
//! carry elisions (`[,,]` — holes that read back as `undefined`), and
//! admitting one buys nothing that the pushes below do not.
//! * **E2 — element provenance.** Every write into `A` is
//! `Expr::ArrayPush { array_id: A }` whose value is `new C(...)` — inline,
//! `Expr::ArrayPush { array_id: A, .. }` whose value is `new C(...)` — inline,
//! or a local bound by exactly one `Let { init: New { C } }` and pushed
//! exactly once. `Expr::New` covers closed object literals too
//! (`__AnonShape_…`), so records qualify. Perry class constructors cannot
Expand Down Expand Up @@ -1066,7 +1066,9 @@ impl<'a> ArrayWalk<'a> {
self.walk_expr(object);
}
// E2: the one admitted write.
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
if let Some(root) = self.root_of(*array_id) {
let site = match value.as_ref() {
Expr::New { class_name, .. } => PushValue::Fresh(class_name.clone()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ fn push(array_id: u32, value: Expr) -> Stmt {
Stmt::Expr(Expr::ArrayPush {
array_id,
value: Box::new(value),
field_writeback: None,
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ fn push(array_id: u32, value: Expr) -> Stmt {
Stmt::Expr(Expr::ArrayPush {
array_id,
value: Box::new(value),
field_writeback: None,
})
}

Expand Down
4 changes: 3 additions & 1 deletion crates/perry-codegen/src/collectors/refs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ pub fn collect_ref_ids_in_expr(e: &perry_hir::Expr, out: &mut HashSet<u32>) {
walk(index, out);
walk(value, out);
}
Expr::ArrayPush { array_id, value } => {
Expr::ArrayPush {
array_id, value, ..
} => {
out.insert(*array_id);
walk(value, out);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ fn module_with_callback(declare_source_array_param: bool) -> Module {
byte_offset: 0,
cap_args_appended: 0,
}),
field_writeback: None,
}),
Stmt::Expr(Expr::ArrayForEach {
array: Box::new(Expr::LocalGet(ARRAY_ID)),
Expand Down
Loading
Loading