perf(runtime): #6759 C5a — per-key vetting for the class-field inline-guard disable - #6802
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime now tracks declared instance-field names and prototype descriptor keys by hash. The inline class-field guard is disabled only for matching keys, including when class registration occurs after a descriptor install. Tests cover immediate and retroactive disabling. ChangesPer-key inline guard behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClassRegistration
participant RuntimeAllocator
participant DescriptorState
participant DescriptorMutation
ClassRegistration->>RuntimeAllocator: register keys_array and field_count
RuntimeAllocator->>DescriptorState: record declared field names
DescriptorMutation->>DescriptorState: install descriptor with key
DescriptorState->>DescriptorState: compare declared and installed key hashes
DescriptorState-->>DescriptorMutation: update inline guard state
Possibly related PRs
🚥 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 |
proggeramlug
pushed a commit
that referenced
this pull request
Jul 22, 2026
This was referenced Jul 22, 2026
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
proggeramlug
force-pushed
the
perf/6798-c3a-stable-shape-identity
branch
from
July 22, 2026 19:44
f2c5148 to
182695f
Compare
added 2 commits
July 22, 2026 21:44
…-guard disable Prototype-level descriptor installs disable the codegen-inlined class-field fast path process-wide. The inline path only compiles DECLARED instance fields, so an install whose key names no declared field of any registered class (babel's defineProperty(C.prototype, method) storm) can never affect it. Vet the key against FNV hashes of all declared field names (harvested in remember_class_keys_array); ordering is covered both ways via a retro-check when a class registers after the install.
proggeramlug
force-pushed
the
perf/6798-c5a-inline-guard-per-key
branch
from
July 22, 2026 19:44
3340446 to
0043552
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/descriptor_state.rs`:
- Around line 147-151: Update test_reset_class_field_inline_guard to also clear
DECLARED_FIELD_NAME_HASHES and PROTO_DESCRIPTOR_KEY_HASHES, preserving the
helper’s deterministic reset behavior for all class-field inline guard state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eb723ff-4101-463a-a8fe-a34636870c9d
📒 Files selected for processing (3)
changelog.d/6802-inline-guard-per-key-c5a.mdcrates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/object/descriptor_state.rs
Comment on lines
+147
to
+151
| #[cfg(test)] | ||
| pub(crate) fn test_reset_class_field_inline_guard() { | ||
| PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED.store(0, Ordering::Relaxed); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test reset helper doesn't clear the new per-key hash sets.
test_reset_class_field_inline_guard() only resets PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED, leaving DECLARED_FIELD_NAME_HASHES (Line 205) and PROTO_DESCRIPTOR_KEY_HASHES (Line 212) accumulated forever across test invocations in the same process. Both are documented as "never pruned." Since the c5a_tests (Lines 1096-1174) rely on this helper for "deterministic unit tests," any future test that reuses a key/field name already inserted by an earlier test (e.g. "c5a_field_x", "c5a_render_method") will get a stale match and silently disable the guard regardless of that test's own setup — a source of order-dependent flakiness that the two current tests happen to avoid only because they use disjoint key names.
🔧 Proposed fix
#[cfg(test)]
pub(crate) fn test_reset_class_field_inline_guard() {
PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED.store(0, Ordering::Relaxed);
+ if let Ok(mut guard) = DECLARED_FIELD_NAME_HASHES.write() {
+ guard.take();
+ }
+ if let Ok(mut guard) = PROTO_DESCRIPTOR_KEY_HASHES.write() {
+ guard.take();
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/descriptor_state.rs` around lines 147 - 151,
Update test_reset_class_field_inline_guard to also clear
DECLARED_FIELD_NAME_HASHES and PROTO_DESCRIPTOR_KEY_HASHES, preserving the
helper’s deterministic reset behavior for all class-field inline guard state.
Contributor
Author
|
Valid — fixed in #6805 (the cfg(test) reset now clears both vetting sets; production monotonicity unchanged). |
proggeramlug
added a commit
that referenced
this pull request
Jul 23, 2026
…t reset (#6805) * test(runtime): clear the C5a per-key vetting sets in the inline-guard test reset (CodeRabbit on #6802) The sets are production-monotonic by design; without clearing them the cfg(test) reset left earlier tests' declared-field / installed-key state visible to later tests reusing a key name, making the disable decision order-dependent. * docs(changelog): changeset for test-reset follow-up --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
that referenced
this pull request
Jul 30, 2026
… gate it (#6837) * chore(warnings): sweep machine-fixable warnings via cargo fix --all-targets Run `cargo fix --workspace --all-targets` now that the workspace test targets compile again, so test-only imports are seen instead of pruned. Clears 87 warnings: 74 unused_imports, 11 function_casts_as_integer (new in rustc 1.95 — casting a function item straight to an integer), plus one each of unused_mut and unused_variables. Two things the tool could not do on its own: - optimized_libs/tests.rs reached build_missing_prebuilt_ext_lib through `super::*`, so cargo fix pruned the re-export it needed. The test now imports the function from no_auto directly. - 49 of the pruned imports were duplicate preambles introduced when commands/compile.rs was split into submodules after #6639. * chore(warnings): drop 150 redundant `unsafe` blocks rustc reports these as `unused_unsafe`: the block wraps code that needs no unsafe context, so the marker hides the sites that do. rustc gives no machine-applicable fix, so this was scripted off the diagnostic byte spans. Where the block only held expressions or statements, the block goes with the keyword. Where it held a top-level `let`, `use`, or item, only the keyword goes and the braces stay — splicing those into the enclosing scope would widen the binding and could silently re-resolve a later name. 106 blocks removed, 44 kept as plain scoping blocks. * chore(warnings): delete two self-declared extern "C" HTTP symbols handle_dispatch.rs declares `js_node_http_res_write` and `js_node_http_res_end` in an `extern "C"` block, but perry-ext-http-server defines both itself (response.rs:1136 and response.rs:1393). Nothing calls the declarations, so rustc reports them as dead functions. Both signatures matched their definitions, so the deletion is a no-op at link time. The other ~62 declarations in that block are used and stay for now; they carry the same hazard — a local declaration of a symbol you also define is never checked against the definition, which is how #6646 shipped an ABI mismatch on `len: i64` vs `u32`. Tracked separately. * chore(warnings): clear 101 dead_code warnings Deletes refactor leftovers — helpers, thunks, fields and constants nothing calls any more. Keeps, with a justified `#[allow(dead_code)]`, the GC verifiers, the `#[cfg(test)]`-only helpers and the FFI keepalives that exist to be reachable from generated code rather than from Rust. No `#[no_mangle]` function is deleted, so no symbol the compiler emits calls to can go missing at link time. The 11 deleted `extern "C"` items are Rust-internal closure thunks addressed by function pointer, which rustc tracks precisely. Rebased from the triage done on 2026-07-18, so three sites needed fixing against current main: - `path::resolve_win32_str` is no longer dead — url/node_compat.rs calls it. Restored. - `class_field_inline_guard_enabled` and `test_reset_class_field_inline_guard` arrived after that triage (#6802). Kept. - The CGContext* declarations in widgets/chart.rs were already removed by #6646. Kept removed. * chore(warnings): clear naming, visibility, unreachable and must-use warnings Four triaged families, rebased onto current main: - non_snake_case: the generated `thunk_<module>_<jsName>` wrappers keep the JS spelling on purpose, so the allow sits on the three thunk macros and the node_submodule thunks rather than on each site. Real Rust names renamed. - private_interfaces / private_bounds: widen the leaked types to `pub(crate)` so the signature matches what callers can already reach. - unreachable_patterns: 32 duplicate match arms. Each was checked against the arm that shadows it — all are literal repeats, none had a different body that the first arm was swallowing. - unreachable_code: two `return js_throw(..)` where `js_throw` returns `!`. - sqlite `Connection::set_limit` returned a discarded `Result`: propagated with `?` in connection.rs, and `let _` in dispatch.rs where the limit id is already validated. - unused_doc_comments: 8 `///` comments on statements, which rustdoc drops. - Four dead bindings whose right-hand side is pure, so the binding goes instead of getting an underscore. Conflicts against main resolved in favour of main: perry-hir switched to `crate::types::Type`, and object/mod.rs moved the transition cache into `RuntimeState`. * chore(warnings): clear the last 65 warnings — host scope is now clean - objc2 deprecations: 17 `msg_send!` invocations were missing the comma between arguments, plus one `Retained::cast` swapped for the `cast_unchecked` the sibling widgets already use. - Vacuous glob re-exports: `pub use child::*` where every item in the child is `pub(super)` re-exports nothing, which is what rustc was reporting. Narrowed each to the visibility it actually has — two to `pub(crate)`, six to a plain `use` — instead of silencing the lint. - `native_proof_*` integration tests share one HIR builder toolkit and each file drives a subset, so the allow sits on the file with that reason. - issue_4914_cluster_port_sharing.rs: its only test is gated to non-macOS unix; the helpers and imports now carry the same gate. - `duplex_allow_half_open_defaults_true_and_honors_false_option` had no `#[test]`, so it had never run. Added. - `test_seed_class_parent_closure_root` existed twice, both writing the same `CLASS_PARENT_CLOSURES` static. Deleted the unreachable copy. - `WIDE_KEY_INDEX_CAPACITY` is a leftover of the 4-entry LRU that #6759 C1 replaced with shape records. - crash_log.rs took a shared reference to a `static mut` inside a signal handler; now `&raw const`. - Two dead stores removed (`idx`, `adjusted_args_storage`). The third, `done` in publish, is kept with a comment: rustc is right that the store is never read, and that means the reconnect guard it feeds can never fire — a protocol question, not a lint one. - Four test names de-camel-cased, one duplicated `#[test]` removed. `cargo check --workspace --all-targets` now reports zero warnings for the host-compatible scope. The cross-host UI crates (ios/tvos/watchos/visionos/ android/windows/gtk4) cannot be checked from macOS and are untouched. * chore(warnings): clean the reduced-feature scope too `perry` depends on perry-runtime with `default-features = false`, so `cargo clippy -p perry --bins` — the product leg in CI — compiles a runtime where regex-engine, diagnostics and temporal are off. Eleven items are live under the workspace feature set and dead under that one, and the workspace check never saw them. Each is now gated at the item rather than silenced: - `array_named_props_install_fresh` and `regex::utf16::utf16_index_to_byte` are called only from regex-engine modules; both, and the array re-export, carry that feature (the cross-gate shape regex/utf16.rs already documents). - Four `fs::dir_glob_watch::glob` imports serve regex-engine-gated code and now match the `PathBuf` import next to them. - The two `AllocatorMaintenance*` enums and `TemporalLocaleCtx` are built from `diagnostics` and `temporal` code respectively, so the allow applies only when that feature is off. - publish's `done` allow moved to the function: a statement-level attribute does not affect `unused_assignments`. Four scopes now report zero warnings: `--workspace --all-targets`, `-p perry --bins`, `-p perry-runtime --no-default-features`, and `-p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-static`. * ci: gate rustc warnings with -D warnings Nothing stopped a PR from adding a warning: `lint` runs only `cargo fmt --check`, and `clippy` exits non-zero only on the deny-level lints in `[workspace.lints]`. The 96 unused imports this branch removed include 49 that reappeared within four days of #6639, from a file split that copied whole import preambles into each new submodule — the gate is what stops that. Two legs, because they compile different code. `perry` depends on perry-runtime with `default-features = false`, so the product leg sees a runtime with regex-engine, diagnostics and temporal off. The workspace leg passes `--all-targets` so test and bench targets count. Separate from the clippy job on purpose: clippy's warn-level lints stay informational, rustc's do not. perry-ui-macos sits in the excluded scope (this runs on ubuntu), so its warnings are not gated here. * docs(changelog): key the warnings-sweep fragment to #6837 * chore(warnings): fix float_literal_f32_fallback under rustc 1.97.1 CI runs `dtolnay/rust-toolchain@stable`, which is 1.97.1 on the runners; the local default stable here is 1.97.1's predecessor, 1.95.0, which does not have this lint yet. `length(1.0)` in the TUI layout pass inferred `f32` by fallback rather than by the `From<f64>` bound, which rustc is phasing out (rust-lang/rust#154024). Spelling the literal `1.0_f32` says what was already meant. It was the only occurrence. All four scopes re-checked under 1.97.1 report zero warnings. * chore(warnings): silence the four warnings that only appear on Linux The macOS host cannot see these; the first CI run on this branch found them. `commands::sandbox_profile` is the #506 MVP, macOS-only by design — its only caller is inside `#[cfg(target_os = "macos")]`, so off macOS all three of its functions are unreachable. The module declaration now carries the same gate as its caller instead of compiling into a build that can never call it. Verified by flipping both cfgs to a target this host is not, and checking: no errors, no warnings. The Linux linker branch in `platform_cmd.rs` binds `let mut c`, but only the `#[cfg(not(target_os = "linux"))]` cross-compile block mutates it, so building on Linux the `mut` is dead. Scoped allow on the binding, which does apply to `unused_mut` — verified by flipping the target and watching the warning disappear. * fix(runtime): restore `use std::path::Path` on non-unix targets The import block read: #[cfg(unix)] use std::os::unix::fs::{MetadataExt, PermissionsExt}; #[cfg(unix)] use std::os::unix::io::AsRawFd; use std::path::Path; `cargo fix` deleted the unused `AsRawFd` line and left its `#[cfg(unix)]` behind, which then applied to the `Path` import below it. Every unix host still compiled; Windows failed with four `cannot find type Path` errors. Swept the whole diff for the same shape — an attribute that outlived the item it was written for — and this was the only one. The other new cfg/import adjacencies are deliberate. * fix: address warnings sweep review * fix(warnings): clean regressions after main sync * fix(workspace): restore container compose default build * fix(gc): poll after allocating loop controls * fix(warnings): remove redundant iterator unsafe block * fix(warnings): remove redundant iterator unsafe block --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C5a of #6759 / #6798 (the "typed_feedback exactness" family, first rung). Stacked on #6801 (Phase C3a).
Problem
disable_class_field_inline_guard_for_targetflips the process-widePERRY_CLASS_FIELD_INLINE_GUARD_DISABLEDkill switch on ANY descriptor install targeting a class prototype orObject.prototype. Babel-compiled classes install every method viadefineProperty(C.prototype, name, …)— so the first babel class definition permanently pushes everythis.fieldaccess in the process from the inlined shape-guard fast path onto the fulljs_typed_feedback_class_field_{get,set}_guardcall. The code itself marked per-key granularity as follow-up work; this is that follow-up. Directly targets the #6759 babel-class-init acceptance micro.What changed
disable_class_field_inline_guard_for_targetnow takes the installed key and vets it againstDECLARED_FIELD_NAME_HASHES— FNV hashes of every declared instance-field name, harvested byremember_class_keys_arrayat class registration (the runtime already walks each class's keys array there).PROTO_DESCRIPTOR_KEY_HASHES, andnote_declared_instance_field_nameretro-checks new field names against it — a class registering late still triggers the disable for previously-installed matching keys.Validation
Full
cargo test -p perry-runtime --lib -- --test-threads=1green (1447 passed), including two new tests: method-key installs keep the guard enabled while declared-field-key installs disable it, and the late-registration retro-check.Summary by CodeRabbit
Performance
Tests