-
-
Notifications
You must be signed in to change notification settings - Fork 159
fix(ci): list buffer/typed-array constructors as poll-capable #8134
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| **Fixed: the `gc-root-dominance` nightly, red since #8120 — buffer/typed-array constructors were missing from `POLL_CAPABLE_RUNTIME`.** | ||
|
|
||
| `--audit-poll-reach` exists to catch exactly one shape (#7616): a symbol whose | ||
| result the checker tracks as a heap value (`ALLOC_RE`) that calls something | ||
| already known to re-enter JS or run a moving minor, without itself being | ||
| listed. A window whose only collection point is such a call classifies | ||
| `MOVING: no`, so every `--moving-only` arm — which is every gated arm, in all | ||
| four corpus × lowering modes — silently drops it. | ||
|
|
||
| Five constructors reach an element read that way, and the audit named them | ||
| with their edges: | ||
|
|
||
| ``` | ||
| js_uint8array_new -> js_typed_array_get, js_uint8array_from_array | ||
| js_typed_array_new_from_array -> js_array_get_f64 | ||
| js_buffer_from_array -> js_array_get_f64 | ||
| js_buffer_from_value -> js_buffer_from_array | ||
| js_buffer_alloc_fill_value -> js_buffer_from_value | ||
| ``` | ||
|
|
||
| The reads are the reach proof: a source element can be an accessor or a Proxy | ||
| `get` trap — user JS — and the per-element loop allocates the destination as | ||
| it goes, so `Buffer.from(arr)` / `new Uint8Array(arr)` are collection points | ||
| like any other element-reading builtin. All five are now listed, which both | ||
| greens the audit and un-drops the windows they gate. | ||
|
|
||
| Validated locally against built runtime archives: `--audit-poll-reach` goes | ||
| from exit 2 (five pairs) to exit 0 (`no ALLOC_RE symbol reaches a poll-capable | ||
| one unlisted`), converging in one pass — listing these five surfaces no | ||
| further unlisted callers. | ||
|
|
||
| Widening this set is one-sided: it can only make windows VISIBLE that the | ||
| `--moving-only` arms previously dropped, so a corpus arm can newly report a | ||
| finding it was blind to. That is the point of the change, and it is also the | ||
| way it could newly exceed a budget — the four corpus × lowering gates in CI | ||
| are what confirm the budgets still hold. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1203,6 +1203,25 @@ def is_collecting(callee): | |
| "js_array_sort_default", "js_array_sort_with_comparator", | ||
| "js_array_map", "js_array_filter", "js_typed_array_for_each", | ||
| "js_array_reduce", "js_json_stringify", | ||
| # Buffer / typed-array construction FROM another collection. Each of these | ||
| # is matched by ALLOC_RE (its result is a heap value the checker tracks) | ||
| # AND reaches an element read that is already poll-capable, so `--audit- | ||
| # poll-reach` reported the pair and refused to let a window whose only | ||
| # collection point is one of them classify `MOVING: no`: | ||
| # | ||
| # js_uint8array_new -> js_typed_array_get, js_uint8array_from_array | ||
| # js_typed_array_new_from_array-> js_array_get_f64 | ||
| # js_buffer_from_array -> js_array_get_f64 | ||
| # js_buffer_from_value -> js_buffer_from_array | ||
| # js_buffer_alloc_fill_value -> js_buffer_from_value | ||
| # | ||
| # The reads are the reach proof: a source element can be an accessor or a | ||
| # Proxy `get` trap, i.e. user JS, and the per-element loop allocates the | ||
| # destination as it goes. `Buffer.from(arr)` / `new Uint8Array(arr)` are | ||
| # therefore collection points like any other element-reading builtin. | ||
|
Comment on lines
+1218
to
+1221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Keep the allocation rationale accurate in both locations. The supplied array-copy implementations snapshot source elements and allocate the destination afterward. Element processing can still execute user JavaScript and allocate, so the poll-capable classification is valid. The current statement that the destination is allocated per element is inaccurate.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| "js_uint8array_new", "js_typed_array_new_from_array", | ||
| "js_buffer_from_array", "js_buffer_from_value", | ||
| "js_buffer_alloc_fill_value", | ||
| "js_string_replace_regex_fn", "js_string_replace_string_fn", | ||
| "js_string_replace_all_regex_fn", "js_string_replace_all_string_fn", | ||
| "js_promise_run_microtasks", "js_gc_loop_safepoint", | ||
|
|
||
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
Add the affected file path to the changelog fragment.
The entry names the five runtime symbols but does not name the changed checker file. Add
scripts/gc_root_dominance_check.pyso the release note is traceable.Based on learnings, changelog fragments should include a long-form root-cause explanation, affected file paths, and validation notes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Source: Learnings