chore: land #10860 — inherited-read cache never primed (v0.5.1628) - #10865
Merged
Merged
Conversation
… it exists to serve #10834 is live in main (train 247, v0.5.1626) and makes inherited reads SLOWER than before it. Same binary, one environment variable apart: | fixture | `PERRY_INHERITED_IC=0` | cache on | | |---|---|---|---| | 8 `Object.create` receivers via an array | 1525.00 | 1600.00 | **+75** | | one `Object.create` receiver, no own keys | 1375.00 | 1481.00 | **+106** | The cache was pure overhead: the probe ran on every read, never served, and the chain walk proceeded unchanged. The counters say why, and they rule out the obvious guess. All four inherited counters read ZERO on the single-receiver fixture — including `declines` — so the prime was never CALLED, not merely refused. Two independent defects: ## A. The prime site is gated on the wrong miss reason `get_field_ic_miss_impl` primes only under `matches!(miss_reason, R::NotOwn)`. A receiver with no keys array reports `ObjectNoKeys` and returns from an earlier arm, several hundred lines before the prime. `Object.create(p)` with nothing of its own is exactly that shape, and it is the most common inherited-read receiver there is. `ObjectNoKeys` means the object has NO own properties at all, so "the key is not an own property" — the precondition the prime needs — holds there MORE strongly than it does under `NotOwn`. The fix primes in that arm and then continues past the cache rather than through it, so the lookup at the top of the function is not repeated. ## B. The slot index ignored the class id `js_object_create` mints a FRESH synthetic class id on every call, so N receivers built by `Object.create(p)` have N different class ids and ONE identical shape. `entry_index` hashed only (shape, key), so all N landed in the same direct-mapped slot and evicted one another. An entry compares `recv_class_id`, so every read missed, re-walked and re-primed: inherited: hits=0 primes=6295655 (ten million reads, eight receivers) A full chain walk PLUS an entry write per read. The fix hashes the class id into the index, so the eight receivers occupy eight slots. ## Result | fixture | main-247 | this | node | |---|---|---|---| | 8 receivers via array | 1600.00 | **494.00** | 19.1 | | single keyless receiver | 1481.00 | **427.00** | 9.0 | `perf stat -x, -e instructions:u`, min of 3, fitted 500 k -> 5 M, two trees whose binaries `cmp` different, output identical to node on both. Counters after: `hits=50108984 primes=1` for the keyless receiver and `hits=57043013 primes=8` for the eight — exactly one prime per receiver, then hits. So this is not a repair to parity; it is the win #10834 was supposed to deliver, on the shapes it was missing entirely. ## Why the original measurement missed both #10834's fixtures give the receiver an own property and mutate it in the loop (`O.x = k`, added to keep the loop honest against node's optimiser). That one incidental detail puts the read on the `NotOwn` path, so defect A never fires, and uses a single receiver, so defect B never fires. On that shape the cache genuinely is a 43% win — 2246 off, 1264 on — which is why the reported numbers were real and generalised badly. ## Tests Two runtime tests, driven through `js_object_get_field_ic` — the real entry the compiled code calls — because both defects live in the miss handler's routing and a test that calls the cache's own functions cannot see either. Against this commit with the two source fixes reverted and the tests kept: a_receiver_with_no_own_keys_is_cached panicked: a keyless receiver never reached the prime, so the cache can never serve this shape and its probe is pure overhead on every read several_object_create_receivers_do_not_evict_each_other panicked: primed 64 times for 8 receivers: every read is re-priming, so the site pays a full chain walk AND an entry write per read `cargo test -p perry-runtime -- --test-threads=1`: 4162 passed, 0 failed.
#10860's prime call sits inside get_field_ic_miss_impl's existing unsafe block (ic_miss.rs:874), so its own unsafe is unused_unsafe. CI's warnings job runs -D warnings and would have rejected the tree.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✨ 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 |
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
2 participants
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.
Lands #10860 — a regression fix for #10834, which is live in
maintoday and makes inherited reads slower than before it.Carried as a train rather than merging the PR head directly, because the PR as submitted would have reddened
main:RUSTFLAGS="-D warnings" cargo checkfailed. The prime call sits insideget_field_ic_miss_impl's existingunsafeblock (ic_miss.rs:874), so its own nestedunsafeisunused_unsafe. CI'swarningsjob runs-D warnings. Fixed here.cargo fmt --all -- --checkwas red in two files. Fixed.changelog.d/fragment. Added.The fix
Two defects made the cache pure overhead — the probe ran on every read, never served, and the chain walk proceeded unchanged.
A. The prime was gated on
R::NotOwn. A receiver with no keys array reportsObjectNoKeysand returns from an earlier arm, hundreds of lines before the prime — andObject.create(p)with nothing of its own is exactly that shape, the most common inherited-read receiver there is.ObjectNoKeysmeans no own properties at all, so the prime's precondition holds there more strongly than underNotOwn.B.
entry_indexhashed only (shape, key).js_object_createmints a fresh synthetic class id per call, so N receivers fromObject.create(p)have N class ids and one shape; all N shared a direct-mapped slot and evicted each other, while the entry compare onrecv_class_idmade every read miss, re-walk and re-prime —hits=0 primes=6295655over ten million reads.Object.createreceivers via an arrayAudit
entry_indexhas exactly one definition and three callers, all updated consistently — a missed site would desync prime from lookup and silently reinstate the miss. TheObjectNoKeysarm primes and then falls through viaget_field_by_name_past_inherited_cache, so the top-of-function lookup is not repeated, and!inherited_declinedstill respects hook A's decline.Both regression tests pass in release (
a_receiver_with_no_own_keys_is_cached,several_object_create_receivers_do_not_evict_each_other); the PR reports them proven non-vacuous — reverting the source fixes makes each panic with a specific message.Gates on the assembled tree:
check_file_size,raw_handle_debt,gc_runtime_root_holders,addr_class_inventory,shape_descriptor_census,string_payload_access_inventory,gc_rekeyed_key_tablesall rc=0;cargo fmt --all -- --checkclean;cargo check --all-targetsunder-D warningsclean.Expedited at the owner's request — landed on a targeted audit plus the PR's own regression tests rather than a full train sweep, because the regression is live and costs 2442 instructions per read on a keyless three-level chain.