-
-
Notifications
You must be signed in to change notification settings - Fork 161
perf(gc): walk an inline slot mask instead of re-entering the iterator (#10362) #10584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
proggeramlug
wants to merge
4
commits into
PerryTS:main
from
proggeramlug:perf/10362-inline-mask-walk
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| ### Fixed | ||
|
|
||
| - **A relocated non-object owner lost its explicit `[[Prototype]]` (#10493).** `Object.setPrototypeOf` on a receiver that is not meta-capable records the prototype in the residual address-keyed registry (#9304), and relocation owes that entry two things. Neither happened outside arrays and ordinary objects: `layout_transfer` reached the rekey only *below* its layout-kind early return, which a `GcLayoutSlotKind::None` cell never passes, and the recorded value was emitted as a child edge from the Array and Object arms alone. So a lazy JSON array, Map, Set, Error, Promise, Date, RegExp, Temporal cell or `dyn_eval` closure silently lost its prototype at its first relocation — correct before a collection, wrong after, exit code 0 and no warning — and the prototype value itself was neither retained nor rewritten. Fixing only the rekey is worse than fixing neither: it turns "prototype lost" into "entry names a stale address", a state measured between the two halves. | ||
|
|
||
| Both obligations now follow the registry's population, stated once in `prototype_chain::residual_prototype_owner_type` (everything except strings, bigints, meta records and compiled regex programs) rather than being wired to two kinds by hand. The rekey runs before the layout-kind return for every owner kind, latch-gated, with the move in a `#[cold]` call; the array-arm and move-hook copies are deleted. The recorded value is emitted ahead of the kind arms, so no arm's early return can skip it. | ||
|
|
||
| Each half has its own sabotage witness: removing the rekey fails at "the registry entry did not follow its owner", and restricting the value visit back to arrays and objects fails at "the recorded prototype still names its pre-collection address". Cost is +0.04% to +0.16% instructions where the registry is never armed, and +0.68% on a fixture that arms it and churns Errors/Maps/Dates — the per-owner cost arrays and objects have always paid. | ||
|
|
||
| Not a regression: the funnel before #10381 returns on the same check. Found from a CodeRabbit review comment on #10381 that landed unactioned; the population turned out to be six kinds wider than the one it named. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| Walk an inline slot mask directly instead of re-entering the slot iterator once | ||
| per slot. `visit_gc_layout_slot_descriptors` called | ||
| `HeapChildSlotIterator::next` for every payload slot; for the common case — a | ||
| `Masked` selection whose mask is `LayoutSlotMask::Inline` — each of those calls | ||
| re-dispatched the selection, re-decoded the mask's niche and rebuilt the limit | ||
| and cursor masks, for about eight instructions of work. | ||
|
|
||
| The mask's set bits are the slot indices, in ascending order, so the arm takes | ||
| the word once and walks it with `trailing_zeros` and `word &= word - 1`. Every | ||
| other selection, including a `Heap` mask (more than 64 payload slots), keeps the | ||
| iterator. The helper carries the iterator's two side conditions with it: the | ||
| one-shot raw-numeric accounting that `next`'s first call performs, and the | ||
| cursor, left at the end so a later `next` yields nothing. The prefix and meta | ||
| edges belong to the caller, which takes them before the payload; the helper | ||
| asserts they are gone rather than arguing it. | ||
|
|
||
| Measured on a control whose pointer fields target DISTINCT objects, because the | ||
| older shared-child control let the collector's one-entry address memo answer | ||
| 83.3% of its classifications against 0.0% on the real fixtures, and so hid the | ||
| cost of everything downstream of that memo. On it, `next` costs 75.8 of the | ||
| 417.1 instructions a pointer-slot visit costs, and the walk removes 69.7 of | ||
| them. On the same control the shared-child version reports 75.9 — the iterator's | ||
| own cost is what the blind control did NOT distort. | ||
|
|
||
| The descriptor walk serves the copying minor, the full mark and the | ||
| remembered-set rebuild. Inclusive instructions for the walk on gc3, exact, by | ||
| caller: copying minor -7.95%, full mark -16.62%, remembered-set rebuild -17.59%, | ||
| dirty scan unchanged. On `oldyoung`, whose masked population is mostly one | ||
| `Heap` mask, the remembered-set rebuild and the dirty-coverage restore each pay | ||
| one failed `take_inline_mask_word` dispatch per visit: +0.25% and +0.33%, about | ||
| two instructions per object visit, against -4.49% on that fixture's copying | ||
| minor and -1.07% on the program. | ||
|
|
||
| Whole program, instructions:u, min of 5: gc3 -6.75%, w20000 -5.86%, w5000 | ||
| -4.90%, w1000 -2.36%, oldyoung -1.06%, and an allocation-only fixture flat to | ||
| 298 instructions in 320 million. No fixture regresses in instructions, peak RSS | ||
| or max GC pause. | ||
|
|
||
| The equivalence between the walk and the iterator is a property, and is tested | ||
| as one: identical index sequences for every mask word (empty, one bit at each | ||
| end, full width, both alternations, and 64 pseudo-random words) crossed with | ||
| every live slot count from 0 to 128, with a sabotaged twin that drops the mask's | ||
| top bit and must be caught, plus a real collection whose only young child hangs | ||
| off the highest masked slot and its own sabotaged twin. | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the slot-count coverage match this claim.
Line 42 states that the test covers every slot count from 0 through 128.
slot_counts()covers only 14 selected counts.Use
(0..=128).collect()inslot_counts(). Alternatively, describe the coverage as selected boundary counts.🤖 Prompt for AI Agents