diff --git a/changelog.d/7288-path-dependent-class-field-store.md b/changelog.d/7288-path-dependent-class-field-store.md new file mode 100644 index 0000000000..189ea4625c --- /dev/null +++ b/changelog.d/7288-path-dependent-class-field-store.md @@ -0,0 +1,83 @@ +**Fixed** a byte-identical `.ts` file compiling to a **46x slower** object depending +on *where on disk it lives*. `benchmarks/suite/09_method_calls.ts` ran 83 ms +compiled inside the Perry checkout and 3,762 ms compiled anywhere else, and +`benchmarks/results/public-node-bun-v1.json` published the fast number (79 ms) +because `benchmarks/compare.sh` does `cd benchmarks/suite` first. A user +compiling the same file in their own project got the slow arm. + +**The discriminator is strict mode, resolved by an upward directory walk.** +`perry_parser::file_is_in_esm_package_context` walks up from the source file for +the nearest `package.json`; `"type": "module"` makes an ambiguous-extension file +(`.ts`/`.js`) an ES module, and module code is strict code +(`lower_module_fn::module_has_strict_mode`, #6542). Perry's own root +`package.json` is `"type": "module"`, so *every* file inside the checkout is +strict and every file outside it — with no `package.json` above — is sloppy. +That determination is correct and matches Node; what was wrong was how much +codegen hung off it. + +`put_value_static_property_fast_path` (`expr/proxy_reflect.rs`) barred sloppy +code from the entire class-field store route with three `if !strict { return +None; }` bails. The stated reason (#6542) is real but narrow: that route's +terminal fallback is `js_class_field_set_fallback` → +`js_object_set_field_by_name`, which throws unconditionally on a non-writable +slot — correct for strict `PutValue`, wrong for sloppy, where a rejected write +is a silent no-op. The bail discarded the **fast** arm to fix the **fallback** +arm. Sloppy `this.value = this.value + 1` fell all the way to +`js_put_value_set_dyn_ic`, a runtime call per iteration. + +The fast arm never needed the bail. The #5093 inline precheck +(`emit_class_field_inline_precheck`) already rejects every receiver whose store +could be *rejected* — `OBJ_FLAG_FROZEN`, `OBJ_FLAG_HAS_DESCRIPTORS`, a +mismatched class id or keys token, a cleared typed-layout-intact bit — and every +value that is not a plain finite number, and the process-global gate is flipped +by any prototype-level descriptor install naming a declared field. A store that +reaches the raw slot is one that could not have been rejected in *either* mode, +so the fast arm is mode-independent by construction. + +Sloppy `obj.f = ` on a declared `number` field of a known class now +emits that same precheck and the same raw slot store +(`property_set::try_lower_sloppy_class_field_raw_store`), and routes every miss +to `js_put_value_set(..., strict = 0)` — the sloppy-correct runtime the +surrounding `PutValueSet` lowering already used — instead of the throwing +by-name setter. No runtime change was needed. Scope is deliberately narrow: +raw-f64 (`number`) fields, receiver == target; boxed slots need the layout note +and write barrier the guard-call path emits and stay on the unchanged inline +caches, as do oversized modules that full-outline the whole diamond (#5334). + +`09_method_calls` outside a checkout: **3,762 ms → 81 ms**, matching the +in-checkout arm (80–83 ms) exactly, so the two arms now agree and the published +baseline describes what users actually get. + +**Two related findings worth recording.** + +The issue's `compilePackages` lead was a red herring, and this explains it: +adding a `package.json` carrying a `perry` key *inside* the checkout flipped the +build to the slow arm not because of `compilePackages` but because Node stops at +the nearest package scope — a nested `package.json` without `"type": "module"` +ends the walk at a non-ESM scope. Confirmed directly: `{"name":"x"}` with no +`perry` key at all reproduces the flip (3,835 ms), and `{"type":"module"}` +outside the checkout gives the fast arm (83 ms). + +**The gap suite structurally cannot see this class of bug.** Every +`test-files/*.ts` sits under the repo root's `"type": "module"`, so the whole +corpus compiles strict; `run_parity_tests.sh` already acknowledges this for Node +(it retries failed import-free globals fixtures as `.cts` to get script +semantics) but the compiler side has no sloppy arm under test. Any codegen +predicate keyed on `strict` is exercised in one state only. + +Verified: a 20-case sloppy differential (inheritance chains, shadowed subclass +fields, accessors, string fields, `null`/`undefined`/boolean/object/BigInt +stores into a `number` slot, `2**53` / `-1e308` / denormal boundaries, +`preventExtensions`, `delete` then re-add, aliased writes, a 100k-iteration +megamorphic loop over 50 instances, `Object.freeze` *mid-loop*, and enumeration +order) is **byte-identical to Node 26.5.1 and to the pre-change compiler**, with +150 `class_field_sloppy_set` blocks in the emitted IR proving the new arm is +live; a separate 8-case frozen/non-writable/prototype-accessor probe matches +Node in both sloppy and strict mode. 24/24 `test_gap_class*` / `test_gap_object*` +tests byte-match Node. `cargo test -p perry-codegen --lib` 633 passed; +`native_proof_regressions` has the identical 4 pre-existing failures before and +after (249 passed vs 248, the extra pass being the new test). The strict arm's +emitted IR is unchanged on 22 of 25 in-checkout files; the 3 that differ are +**self-nondeterministic** — the unmodified compiler produces three different +hashes across three runs of the same input (#7303) — and their output still +matches Node. diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index 4e2558084b..7e42451d6c 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -43,6 +43,168 @@ fn class_has_computed_runtime_members(ctx: &FnCtx<'_>, class_name: &str) -> bool .is_some_and(|class| !class.computed_members.is_empty()) } +/// #7288: the SLOPPY-mode arm of the #5093 class-field raw-f64 store. +/// +/// `put_value_static_property_fast_path` bars sloppy code from the whole +/// class-field route (#6542) because that route's terminal fallback is +/// `js_object_set_field_by_name`, which throws unconditionally on a +/// non-writable slot — correct for strict `PutValue`, wrong for sloppy, where a +/// rejected write is a silent no-op. +/// +/// That bail is far wider than the hazard, and the width is user-visible: an +/// identical `.ts` file compiles to a 46× slower object depending only on +/// whether an upward walk from the source finds a `package.json` with +/// `"type": "module"` (which makes the module ESM, hence strict). Inside the +/// Perry checkout it does; in a user's scratch directory it does not, so +/// `benchmarks/suite/09_method_calls.ts` measured 83 ms in-tree and 3.8 s +/// anywhere else. +/// +/// The fast arm never needed the bail. The #5093 inline precheck +/// (`emit_class_field_inline_precheck`) already rejects every receiver whose +/// store could be *rejected* — `OBJ_FLAG_FROZEN`, `OBJ_FLAG_HAS_DESCRIPTORS`, a +/// mismatched class id or keys token, a cleared typed-layout-intact bit — plus +/// every value that is not a plain finite number, and the process-global gate +/// is flipped by any prototype-level descriptor install naming a declared +/// field. A store that reaches the raw slot is therefore one that could not +/// have been rejected in either mode, so the fast arm is mode-independent. +/// Only the fallback needed strict-awareness, and this sends every miss to +/// `js_put_value_set(..., strict = 0)` — the sloppy-correct runtime the +/// surrounding `PutValueSet` lowering already uses — instead of the throwing +/// by-name setter. +/// +/// Scope is deliberately narrow: declared raw-f64 (`number`) fields on a known +/// class, receiver == target. Boxed slots need the layout note and write +/// barrier that the guard-call path emits, so they stay on the unchanged +/// sloppy inline caches. +pub(crate) fn try_lower_sloppy_class_field_raw_store( + ctx: &mut FnCtx<'_>, + object: &Expr, + property: &str, + value: &Expr, +) -> Result> { + // Oversized modules full-outline the whole IC diamond into one call + // (#5334 lever B); that outlined runtime has no sloppy variant, so leave + // those modules on the unchanged path. + if crate::codegen::full_outline_ic_enabled() { + return Ok(None); + } + let Some(class_name) = receiver_class_name(ctx, object) else { + return Ok(None); + }; + if class_has_computed_runtime_members(ctx, &class_name) { + return Ok(None); + } + // A compiled setter owns the name; never store into the slot behind it. + // (`class_field_global_index` also rejects accessors anywhere in the + // chain — this is the same check the strict arm makes first, kept so the + // two arms agree on which shapes are eligible.) + if ctx + .methods + .contains_key(&(class_name.clone(), format!("__set_{}", property))) + { + return Ok(None); + } + let Some(field_index) = + crate::type_analysis::class_field_global_index(ctx, &class_name, property) + else { + return Ok(None); + }; + let (Some(&expected_class_id), Some(keys_global_name)) = ( + ctx.class_ids.get(&class_name), + ctx.class_keys_globals.get(&class_name).cloned(), + ) else { + return Ok(None); + }; + // Raw-f64 slots only — see the doc comment. + if !crate::type_analysis::class_field_declared_type(ctx, &class_name, property) + .as_ref() + .is_some_and(crate::typed_shape::type_is_raw_f64_candidate) + { + return Ok(None); + } + + // Operand order mirrors the strict class-field arm below verbatim: the + // assignment reference is evaluated before the RHS, and the receiver's + // relocation across an allocating RHS is handled by the same statepoint + // re-read that arm relies on. + let recv_box = lower_expr(ctx, object)?; + let val_double = lower_expr(ctx, value)?; + + let key_idx = ctx.strings.intern(property); + let key_handle_global = format!("@{}", ctx.strings.entry(key_idx).handle_global); + let field_idx_str = field_index.to_string(); + let expected_class_id_str = expected_class_id.to_string(); + + let (obj_bits, obj_handle, key_box, val_bits, expected_keys) = { + let blk = ctx.block(); + let obj_bits = blk.bitcast_double_to_i64(&recv_box); + let obj_handle = blk.and(I64, &obj_bits, POINTER_MASK_I64); + let key_box = blk.load(DOUBLE, &key_handle_global); + let val_bits = blk.bitcast_double_to_i64(&val_double); + let expected_keys = blk.load(I64, &format!("@{}", keys_global_name)); + (obj_bits, obj_handle, key_box, val_bits, expected_keys) + }; + + let fast_idx = ctx.new_block("class_field_sloppy_set.fast"); + let merge_idx = ctx.new_block("class_field_sloppy_set.merge"); + let fast_label = ctx.block_label(fast_idx); + let merge_label = ctx.block_label(merge_idx); + + // Emits the shape/flags/value precheck and branches to `fast_label` on a + // hit; leaves `ctx.current_block` on the freshly created miss block. + let _miss_label = crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( + ctx, + &obj_bits, + &obj_handle, + &expected_class_id_str, + &expected_keys, + field_index, + true, + Some(&val_bits), + &fast_label, + ); + + // Miss: the strict-aware runtime with `strict = 0`, so a rejected write + // stays a silent no-op exactly as sloppy `PutValue` requires. + { + let blk = ctx.block(); + let _ = blk.call( + DOUBLE, + "js_put_value_set", + &[ + (DOUBLE, &recv_box), + (DOUBLE, &key_box), + (DOUBLE, &val_double), + (DOUBLE, &recv_box), + (I32, "0"), + ], + ); + blk.br(&merge_label); + } + + ctx.current_block = fast_idx; + { + // arm64_32 watchOS: the fields region starts at `size_of::()` + // past the user pointer (24 on 64-bit, 20 on ILP32) — same derivation as + // the strict arm and the runtime setter. + let header_skip = + crate::target_layout::object_header_size_bytes(ctx.target_triple).to_string(); + let blk = ctx.block(); + let obj_ptr = blk.inttoptr(I64, &obj_handle); + let fields_base = blk.gep(I8, &obj_ptr, &[(I64, &header_skip)]); + let field_ptr = blk.gep(DOUBLE, &fields_base, &[(I64, &field_idx_str)]); + // GC_STORE_AUDIT(POINTER_FREE): a guarded raw-f64 class slot holds + // numbers only, and the precheck rejected every value that is not a + // plain finite double, so no write barrier and no layout note are due. + let numeric_value = canonicalize_raw_f64_numeric_store_value(blk, &val_double); + blk.store(DOUBLE, &numeric_value, &field_ptr); + blk.br(&merge_label); + } + + ctx.current_block = merge_idx; + Ok(Some(val_double)) +} + fn lower_runtime_property_set_by_name( ctx: &mut FnCtx<'_>, object: &Expr, diff --git a/crates/perry-codegen/src/expr/proxy_reflect.rs b/crates/perry-codegen/src/expr/proxy_reflect.rs index 82daa08611..cadb930131 100644 --- a/crates/perry-codegen/src/expr/proxy_reflect.rs +++ b/crates/perry-codegen/src/expr/proxy_reflect.rs @@ -1240,6 +1240,28 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { }, ); } + // #7288: sloppy code is barred from the class-field route above + // because that route's fallback throws on a rejected write. The + // FAST arm is mode-independent (its precheck rejects frozen / + // descriptor-bearing receivers and non-number values), so emit it + // here with a sloppy-correct miss path instead of surrendering the + // whole optimization. See + // `property_set::try_lower_sloppy_class_field_raw_store`. + if !*strict { + if let Expr::String(property) = key.as_ref() { + if same_put_value_receiver_expr(target, receiver) + && matches!(target.as_ref(), Expr::LocalGet(_) | Expr::This) + { + if let Some(result) = + super::property_set::try_lower_sloppy_class_field_raw_store( + ctx, target, property, value, + )? + { + return Ok(result); + } + } + } + } if put_value_index_fast_path(ctx, target, key, receiver) { return super::index_set::lower( ctx, diff --git a/crates/perry-codegen/tests/native_proof_regressions.rs b/crates/perry-codegen/tests/native_proof_regressions.rs index 70d28a4b57..993d8a51ac 100644 --- a/crates/perry-codegen/tests/native_proof_regressions.rs +++ b/crates/perry-codegen/tests/native_proof_regressions.rs @@ -13960,6 +13960,118 @@ fn element_to_element_numeric_store_takes_the_inline_guard_tier() { ); } +/// #7288: a SLOPPY-mode `obj.f = ` on a declared `number` field of a +/// known class must take the inline class-field raw store, not a per-write +/// inline-cache miss. +/// +/// The strict/sloppy split is invisible in the source and comes from an upward +/// walk for the nearest `package.json`: `"type": "module"` makes the file ESM, +/// hence strict. So the identical `.ts` file compiled inside the Perry checkout +/// (whose root `package.json` is `"type": "module"`) took the fast arm at 83 ms, +/// and compiled in a user's scratch directory took the slow arm at 3.8 s — a +/// 46x cliff with no diagnosable cause, and the reason +/// `benchmarks/results/public-node-bun-v1.json` reported 79 ms for +/// `09_method_calls` while a user running the same file saw 3.4 s. +/// +/// Asserts the codegen decision, not wall-clock time, and asserts the arm is +/// LIVE (the fast block exists) rather than merely that nothing threw. The +/// sloppy-correct miss path is asserted too: `js_put_value_set` takes a +/// `strict` operand and must receive 0, so a rejected write stays a silent +/// no-op instead of throwing the way the strict route's +/// `js_object_set_field_by_name` fallback would. +#[test] +fn sloppy_class_field_number_store_takes_the_inline_raw_store() { + // The synthetic `Counter` constructor legitimately emits the STRICT + // class-field store (class bodies are always strict), so every assertion + // below is scoped to the probe function that holds the store under test. + fn probe_body(ir: &str) -> &str { + let start = ir + .find("define double @perry_fn_sloppy_class_field_store_ts__probe") + .expect("probe function must be emitted"); + let rest = &ir[start..]; + let end = rest[1..] + .find("\ndefine ") + .map(|offset| offset + 1) + .unwrap_or(rest.len()); + &rest[..end] + } + + fn ir_for(strict: bool) -> String { + let counter = class(217, "Counter", vec![class_field("value", Type::Number)]); + let module = module_with_classes_and_params( + "sloppy_class_field_store.ts", + vec![counter], + vec![param(1, "counter", Type::Named("Counter".to_string()))], + Type::Number, + vec![ + Stmt::Expr(Expr::PutValueSet { + target: Box::new(local(1)), + key: Box::new(Expr::String("value".to_string())), + value: Box::new(Expr::Number(7.0)), + receiver: Box::new(local(1)), + strict, + }), + Stmt::Return(Some(int(0))), + ], + ); + compile_ir_for_module_with_opts(module, empty_opts()).unwrap() + } + + let sloppy_module = ir_for(false); + let sloppy = probe_body(&sloppy_module); + assert!( + sloppy.contains("class_field_sloppy_set.fast"), + "a sloppy number-field store must reach the inline class-field raw \ + store; the #6542 `!strict` bail was wider than the hazard it guarded \ + (#7288):\n{sloppy}" + ); + // The precheck that makes the fast arm mode-independent: it rejects a + // frozen receiver (OBJ_FLAG_FROZEN) and one carrying any property + // descriptor (OBJ_FLAG_HAS_DESCRIPTORS) — exactly the receivers whose + // store strict and sloppy disagree about. + assert!( + sloppy.contains("class_field_inline.deref"), + "the sloppy arm must be fronted by the #5093 shape/flags precheck, \ + which is what makes the raw store safe in sloppy mode (#7288):\n{sloppy}" + ); + // Miss arm: strict-aware runtime, strict = 0. Matched on the CALL, not the + // module's extern `declare` line — every runtime symbol is declared whether + // or not it is reached, so a `contains("@js_put_value_set")` would pass + // vacuously. + let miss_call = sloppy + .lines() + .find(|line| line.contains("call double @js_put_value_set(")) + .unwrap_or_else(|| { + panic!("the sloppy arm's miss must CALL `js_put_value_set` (#7288):\n{sloppy}") + }); + assert!( + miss_call.trim_end().ends_with("i32 0)"), + "the sloppy miss must pass strict = 0, so a rejected write is a silent \ + no-op rather than a throw (#7288):\n {miss_call}" + ); + // The throwing strict fallback must not be reached on this arm. + assert!( + !sloppy.contains("call void @js_class_field_set_fallback"), + "the sloppy arm must not CALL `js_class_field_set_fallback`, whose \ + `js_object_set_field_by_name` throws unconditionally on a \ + non-writable slot (#7288):\n{sloppy}" + ); + + // Negative control: the strict arm is unchanged and keeps its own blocks, + // so the assertions above are testing the new path rather than a rename. + let strict_module = ir_for(true); + let strict = probe_body(&strict_module); + assert!( + !strict.contains("class_field_sloppy_set"), + "the strict arm must keep its existing lowering (#7288):\n{strict}" + ); + assert!( + strict.contains("class_field_set.fast"), + "the strict arm must still take the class-field store fast path — if \ + this fails the test is measuring nothing (#7288):\n{strict}" + ); +} + #[path = "native_proof_regressions/invalidation.rs"] mod invalidation;