perf(runtime): skip dead feedback observation + field-only shape reads (computed-key read now at node parity) - #8983
Conversation
Typed-feedback recording is off by default, and guard_observe and record_fallback_call both early-return in that mode — but the property wrappers had already built the whole Observation to hand them, hashing the key and resolving the receiver's shape first. On an isolated property-read loop js_typed_feedback_object_get_field_by_name_f64 was 10% of self time, nearly all of it that dead work. Apply PerryTS#5094's gate, which the array index wrappers already carry and PerryTS#8951 gave the fast store path: when recording is off, take the underlying op directly. Behaviour is unchanged in both modes — with recording off guard_observe returns contract_valid and the fallback recorder is a no-op, so the wrapper already reduced to exactly this call. Also: object_live_slot_count reads live_inline_slot_count through the shape table's record instead of lifting the whole ~48-byte descriptor to discard all but four bytes. That bound is consulted on essentially every property operation, and shape_descriptor_by_id was 10.1% of the same loop. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe runtime now skips typed-feedback observation setup when tracing is disabled. Shape access reads ChangesProperty access optimizations
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change removes unnecessary property-read work while preserving existing behavior and validation, with the supplied test and differential results passing. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant TypedFeedback
participant TracingState
participant ObjectOperation
TypedFeedback->>TracingState: check tracing enabled
TracingState-->>TypedFeedback: disabled
TypedFeedback->>ObjectOperation: perform property read or write
ObjectOperation-->>TypedFeedback: return operation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description gives detailed optimization context, measurements, and test results, but it does not follow the required template structure. It omits the Summary, Changes, Related issue, Test plan, and Checklist headings and does not state a related issue or checklist status. Resolution Rewrite the description using the repository template. Add the required headings, list the concrete changes, specify a related issue or use "n/a", include the test commands and results, and complete the checklist items. Keep the existing measurement and correctness details under the appropriate sections. Full details: Docstring CoverageExplanation Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 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 |
|
Merged (batched with #8981 and #8982). Both removals are dead work rather than new behaviour — building an One fix pushed: the fragment was named That makes three fragment-naming problems in this batch of three: one named for an issue, one carrying another PR's number, one missing entirely. I filed #8978 about this earlier — the gate validates the filename shape ( Validation — runtime 2780/0, hir 355/0, codegen 1341/0, stdlib 124/0 ( |
1 participant
Two dead-work removals on the property read path, both taken from the top of an isolated read profile.
1. Typed-feedback observation when recording is off. Recording is off by default, and both
guard_observeandrecord_fallback_callearly-return in that mode — but the property wrappers built the entireObservationfirst, hashing the key and resolving the receiver's shape, purely to hand it to functions that discard it.js_typed_feedback_object_get_field_by_name_f64was 10% of an isolated property-read loop, nearly all of it that.The array index wrappers have carried #5094's gate for exactly this reason, and #8951 gave it to the fast store path. The property get/set wrappers never got it.
Behaviour is unchanged in both modes, and provably so: with recording off
guard_observereturnscontract_validunmodified and the fallback recorder is a no-op, so the wrapper already reduced to precisely the underlying call it now makes directly.2. Field-only shape reads.
shape_descriptor_by_idreturnsShapeDescriptorby value, soobject_live_slot_count— the slot bound consulted on essentially every property read and write — lifted the whole ~48-byte record and kept four bytes of it. It now reads the field through the table's record using the same way-cache probe and the same epoch validation.shape_descriptor_by_idwas 10.1% of the same loop.Measurement
Interleaved A/B, min-of-21, built from the exact parent commit and this commit in one run. Node on the same host in brackets:
Means track the mins (22 → 18, 28 → 24, 46 → 42).
Correctness
perry-runtime: 2779 passed / 0 failed, including the 55 typed-feedback tests — that gate changes when observations get recorded, so it is the one to watch.Where the remaining gap is
The pure property read (
o[k], key already built) is now 17 ms against node's 4 ms and is the largest single remaining gap in this area — worth noting that perry's key construction is already faster than node's, so this is the lookup itself.https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
Summary by CodeRabbit
Performance Improvements
Reliability