perf(runtime): restore method-scoped prototype guards - #8672
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds hashed per-method prototype guard invalidation. Code generation and runtime guards pass method slots, prototype mutations invalidate targeted slots, and method resolution skips deleted entries. Tests cover guard preservation, same-name invalidation, deletion, inheritance, and method replacement. ChangesMethod-scoped prototype guard invalidation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR restores method-scoped prototype-guard invalidation so unrelated prototype writes do not unnecessarily disable optimized method calls, while preserving invalidation for affected methods. The documented validation supports merge readiness, and no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ape guards (#8682) Lands three reviewed PRs as one squash: #8674, #8675, #8676. - #8674: compress array forwarding chains (`clean_arr_ptr` multi-hop walk). - #8675: share numeric guards across dynamic add trees. - #8676: pack monomorphic method shape guards. These three were authored as a stack on top of #8672, but their contents touch disjoint files, so they are cherry-picked onto main on their own. #8672 is NOT included: it defines its own `is_bound_native_method_closure_value` (true for any bound native-module export with a non-empty module name), which #8662 superseded on main with the strictly narrower `is_bound_native_constructor_closure_value` (gated on explicit constructor metadata). Those predicates have different truth sets, so the substitution is a behavioural change at every call site and is left to the author to rebase. Also splits `array/tests.rs`, which #8674 pushed over the 2000-line cap, into an `array/forwarding_tests.rs` sibling. Pure relocation. Version bump not included per maintainer policy. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
Holding this one for a rebase — the rest of your stack (#8674, #8675, #8676) landed via #8682, cherry-picked onto The blocker is a genuine semantic collision, not a mechanical conflict. This PR adds to pub(crate) fn is_bound_native_method_closure_value(value: f64) -> bool {
unsafe {
bound_native_callable_module_and_method(value)
.map(|(module, _)| !module.is_empty())
.unwrap_or(false)
}
}Meanwhile #8662 landed on /// True when `value` is a bound native-module *constructor* export. Native
/// constructors and ordinary module functions share `BOUND_METHOD_FUNC_PTR`,
/// so the export's explicit constructor metadata must make the distinction.
pub(crate) fn is_bound_native_constructor_closure_value(value: f64) -> bool {
bound_native_callable_is_constructor_value(value)
}These are not the same function under two names. Yours is true for any bound native-module export with a non-empty module name — constructor or ordinary method. Both comments describe the same goal — making Two smaller mechanical items in the same rebase, both from
The only conflicted file is the |
|
Followed up on my earlier comment — I dug into the call site and I think this unblocks cleanly, with no judgment call needed after all. I was wrong to leave it open-ended; the evidence settles it. Your predicate has exactly one call site, if let Some((module, method)) = unsafe {
native_module::bound_native_callable_module_and_method(parent_value)
} {
if normalize_native_module_alias(&module) == "wasi" && method == "WASI" {
register_class(class_id, crate::wasi::CLASS_ID_WASI);
}
return; // <-- unconditional
}
if is_bound_native_method_closure_value(parent_value) {
return; // <-- line 132
}Now compare the two candidate predicates. Both are built on the same underlying query: // yours
bound_native_callable_module_and_method(value)
.map(|(module, _)| !module.is_empty()).unwrap_or(false)
// main's (#8662)
bound_native_callable_module_and_method(value)
.is_some_and(|(m, p)| is_native_module_constructor_export(&m, &p))Each can only be That means the conflict has no behavioural content: substituting Suggested resolution: drop line 132 and its import entirely, rather than porting either predicate. The comment above it ("Keep the parentless baseline rather than mis-throwing") is already satisfied by the Worth double-checking my reading before you act on it, since dead-code arguments are easy to get subtly wrong — but if you agree, the |
ba12e94 to
2ea037b
Compare
proggeramlug
changed the title
2ea037b to
9a1e57a
Compare
9a1e57a to
e03d82f
Compare
…Intl worklist (#8723) Lands #8672, #8718, #8720 and #8659. #8672's blocker is resolved the way the evidence pointed. Its own `is_bound_native_method_closure_value` is gone; only main's `is_bound_native_constructor_closure_value` remains, and the branch that called it in `parent_static.rs` is deleted. That branch was unreachable under either predicate -- the `if let Some(..) = bound_native_callable_ module_and_method(..)` block directly above returns unconditionally, and both predicates require that same query to be `Some` -- so removing it is behaviour-preserving rather than a choice between two semantics. #8718 (closes #6620) routes `server.listen(path)`, `net.connect(path)` and the `{ path }` overloads through real Windows named pipes and Unix-domain sockets instead of falling back to TCP. #8720 stabilizes native value profile boundaries; #8659 completes the Intl 402 test262 worklist. One fix on top: a changelog fragment for #8718, which had neither one nor a skip-changelog label. #8719 is NOT in this batch -- it conflicts with #8672 on `lower_call/method_override.rs`, which both touch. No version bump. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
Landed on Validated on the merged result: all 30 lint checkers, runtime 2667/0, codegen 1214/0 plus all integration suites clean, transform 93/0. |
1 participant
Summary
later class-semantics integration reinstated the process-global latch.
allowing unrelated prototype keys to leave hot method guards usable.
deletion, and re-creation correctly.
The all-method latch remains as an escape hatch for any future keyless
prototype mutation. Keyed writes use the low 16 bits of the existing FNV-1a
dispatch hash; collisions conservatively retire additional method names.
Root cause
The original scoped implementation in
2897cb521was superseded when theclass-semantics work was integrated, restoring a sticky process-wide
invalidation byte. The unchanged
codehz/ecscomprehensive suite performsprototype activity before its query benchmarks, so every later direct method
guard failed and each entity call entered generic dispatch.
The performance and composition artifacts below were rebuilt with the fix as
one commit on
mainat60f4fabde. The final PR head is rebased on850e6f1d5; that intervening async/RegExp batch does not overlap this patch.Mac mini benchmark
Apple M1 Mac mini, macOS 26.5.1, AC power,
taskpolicy -t 0 -l 0.The frozen source is upstream
codehz/ecscommit58d729682eeb88d7361796de0420f3d673e27479, benchmark10k entities: forEach query with accumulation, repeat 256, two warmups andsix measured rounds per process.
An exact-parent 11-pair alternating cohort at
8224d879ameasured:Median paired improvement was 91.26%, with 11/11 candidate wins and 22/22
process semantic oracles. The admitted 60-s quiet gate stayed between 4.10%
and 16.60% active CPU. Executable SHA-256 values were
f6532a88ad6635cdbd8cfb72918b9587a9c85c907aaa72a7c44cfbb5d5262614(control) and
2aa1de3236aa37958c914353f737da3b26f7f803d09aae5b2b36b6734c6e4477(candidate).
After rebasing, a fresh current-main candidate was rebuilt and hashed as
52b96f3d8f22fa391310ea889b88d51a17a2c52765d5696574aa707916bc54c2.A quiet three-pair Mac screen against Node 26.5.1 measured 0.615689 ms/op for
Perry and 0.109858 ms/op for Node, with all six process oracles passing.
Perry therefore remains 5.60x slower in this workload; this PR is a large
regression repair, not a parity claim.
Validation
cargo check -p perry-codegen -p perry-runtimeisolation, same-name invalidation, and generic replacement fallback
deletion, and same-name writes on another class; byte-identical normally and
under forced verified-evacuation settings on the rebased compiler
50,005,000checksum locally and onthe Mac mini with the rebased compiler
cargo fmt --all -- --checkgit diff --checkThe full ECS executable still exposes the existing forced-evacuation verifier
panic. The exact parent reproduces the same seventh-copying-minor failure and
signature under the identical command; the focused fixture is green under
those flags. This PR does not claim to fix that separate baseline GC defect.
Summary by CodeRabbit
Performance
Bug Fixes
Tests