Skip to content

perf(runtime): fast-path ordinary native property Get - #10248

Closed
proggeramlug wants to merge 2 commits into
mainfrom
perf/native-property-get
Closed

proggeramlug wants to merge 2 commits into
mainfrom
perf/native-property-get

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Runtime Get on ordinary objects currently repeats key copies, handle scopes, and exotic/descriptor probes even when it can return an own or prototype data slot without calling JavaScript. Add a conservative classified lookup shared by Reflect.get, the named-field ABI, and the native borrowed-name getter. Reuse a canonical key for JSON.stringify's toJSON lookup. Every unproven case keeps the existing slow path; no new GC roots or raw-pointer caches.

Linux perf stat -e instructions:u, medians of three runs against main 9fda98df68, with pinned Node stdout checks:

Probe Instruction reduction
Reflect.get own data 71.24%
Reflect.get prototype data 77.66%
Positive thenable assimilation 12.36%
Inherited toJSON 42.93%
User iterator control 0.05% (neutral)

Baseline measurements preceded runtime edits. Final instruction-sampled DWARF profiles had no lost samples. Build/binary hashes, raw counts, folded stacks, and scope decisions are in the measurement and validation report and its evidence directory.

Validation:

  • Runtime unit suite: 3,803 passed, 4 ignored; regex-off check, fmt, and diff checks pass.
  • Eight fast/forced-slow tests; 42-line Node parity file passes on Linux for base and candidate.
  • Three source mutants fail the intended assertions: remove the fast path, ignore the accessor Bloom guard, ignore an exotic own expando. Restored tests pass; logs are committed.
  • Full lint replay and clippy compared with base. Custody/inventory ratchets pass. Existing freshness, warnings, and API-doc drift failures have base evidence; no new clippy diagnostics.
  • GC ratchet: all 14 probes match Node; all 126 shared-CI metric medians equal a fresh same-host base. The checker passes that A/B comparison. The older repository pin fails on both revisions and remains unchanged.
  • Full GC PR matrix: 428 PASS, 167 UNVER, seven failures confined to the HTTP/2 callback fixture; every required arm passes aggregate liveness. All seven failures and counters reproduce exactly on the unchanged base. Details are in the report.

Ready for review and the merge train. The workspace version remains for the train's version-bump commit. GitHub runners are unavailable; the attached local replay is the validation evidence.

Summary by CodeRabbit

  • Performance

    • Improved property access for ordinary own and inherited data properties.
    • Reduced instruction counts by approximately 71–78% for Reflect.get, 12% for positive thenable handling, and 43% for inherited toJSON serialization.
    • Preserved existing behavior for accessors, proxies, class statics, and other unsupported cases.
  • Bug Fixes

    • Improved compatibility across property reads, prototypes, symbols, frozen objects, typed arrays, built-ins, and JSON serialization.
  • Tests

    • Added comprehensive parity, mutation, benchmark, and runtime validation coverage.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: eb966347-9ea7-4674-bcb7-3b35042503e1

📥 Commits

Reviewing files that changed from the base of the PR and between eb13fa1 and 6fd965a.

⛔ Files ignored due to path filters (7)
  • benchmarks/native_property_get/evidence/api-docs-drift.patch.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/clippy-logs.tar.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/fault-logs.tar.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/gc-ratchet-receipts.tar.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/gc-stress-receipts.tar.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/measurement-receipts.tar.gz is excluded by !**/*.gz
  • benchmarks/native_property_get/evidence/validation-logs.tar.gz is excluded by !**/*.gz
📒 Files selected for processing (33)
  • benchmarks/native_property_get/README.md
  • benchmarks/native_property_get/REPORT.md
  • benchmarks/native_property_get/evidence/base-gate-comparison.json
  • benchmarks/native_property_get/evidence/baseline-build.json
  • benchmarks/native_property_get/evidence/baseline.json
  • benchmarks/native_property_get/evidence/candidate-build.json
  • benchmarks/native_property_get/evidence/candidate.json
  • benchmarks/native_property_get/evidence/clippy-diff.json
  • benchmarks/native_property_get/evidence/faults.json
  • benchmarks/native_property_get/evidence/gc-base-runtime.sha256
  • benchmarks/native_property_get/evidence/gc-ratchet-ab.json
  • benchmarks/native_property_get/evidence/gc-stress-base-http2.json
  • benchmarks/native_property_get/evidence/gc-stress.json
  • benchmarks/native_property_get/evidence/parity.stdout
  • benchmarks/native_property_get/evidence/runtime-source-sha256.json
  • benchmarks/native_property_get/evidence/validation.json
  • benchmarks/native_property_get/faults.py
  • benchmarks/native_property_get/iterator.ts
  • benchmarks/native_property_get/measure.py
  • benchmarks/native_property_get/reflect_own.ts
  • benchmarks/native_property_get/reflect_proto.ts
  • benchmarks/native_property_get/thenable.ts
  • benchmarks/native_property_get/tojson.ts
  • changelog.d/10248-native-property-get.md
  • crates/perry-runtime/src/json/stringify.rs
  • crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs
  • crates/perry-runtime/src/object/mod.rs
  • crates/perry-runtime/src/object/native_get.rs
  • crates/perry-runtime/src/object/native_get/tests.rs
  • crates/perry-runtime/src/proxy/reflect.rs
  • crates/perry-runtime/src/value/dynamic_object.rs
  • scripts/gc_runtime_root_holders.json
  • test-files/test_gap_native_property_get.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Native property Get optimization

Layer / File(s) Summary
Native lookup implementation
crates/perry-runtime/src/object/...
Adds guarded ordinary-data lookups with key, shape, class, accessor, object, and prototype checks. Unsupported cases return to the existing path.
Runtime lookup integration
crates/perry-runtime/src/object/field_get_set/..., crates/perry-runtime/src/proxy/reflect.rs, crates/perry-runtime/src/value/dynamic_object.rs, crates/perry-runtime/src/json/stringify.rs
Uses the native lookup from named-field, Reflect, dynamic-object, and JSON toJSON reads.
Parity and mutation validation
crates/perry-runtime/src/object/native_get/tests.rs, test-files/test_gap_native_property_get.ts, benchmarks/native_property_get/faults.py, scripts/gc_runtime_root_holders.json, benchmarks/native_property_get/evidence/faults.json, benchmarks/native_property_get/evidence/parity.stdout
Adds fast-versus-slow tests, JavaScript parity cases, mutation checks, and test-only root metadata.
Benchmark harness and evidence
benchmarks/native_property_get/*, changelog.d/10248-native-property-get.md
Adds five probes, instruction measurement, benchmark reports, build and source hashes, GC results, validation records, and a changelog entry.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Reflect or runtime Get caller
  participant NativeGet as native_get
  participant ExistingGet as existing Get dispatch
  Caller->>NativeGet: attempt ordinary data lookup
  NativeGet-->>Caller: return data-slot value when eligible
  NativeGet->>ExistingGet: fall through for unsupported or unproven cases
  ExistingGet-->>Caller: perform existing property-read behavior
Loading

Merge Risk: ⚪ Minimal · up to 6fd96

No merge-blocking issue remains identified for this change.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 15 files. (17 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description provides detailed implementation, performance, and validation information, but it does not follow the repository template. It omits the required section headings, related issue entry, … Reformat the description using the repository template. Add Summary, Changes, Related issue, Test plan, Screenshots / output, and Checklist sections. State the related issue or use "n/a", and complete the applicable checklist items.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: adding a fast path for ordinary native property Get operations.
Full details: Docstring Coverage

Explanation

Docstring coverage is 53.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 15 files. (17 skipped: 17 unsupported.)

Full details: Description check

Explanation

The description provides detailed implementation, performance, and validation information, but it does not follow the repository template. It omits the required section headings, related issue entry, checklist, and explicit test-plan checklist.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/native-property-get

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed in merge train #10261: #10261. The merged main tree matches the validated train, and the fresh-head patch audit confirms the changes arrived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant