codegen: inline plain-array pop tier with js_array_pop_f64 as the fallback

[proggeramlug]

What

arr.pop() on an Array-admitted receiver — an erased class field (this.packed.pop(), the NativeMethodCall { array, pop } route) or a claimed-array local/parameter (lower_array_method) — now emits an inline plain-array tier (expr/array_pop.rs) with js_array_pop_f64 kept as the fallback. The inline admission mirrors the runtime fast path's decision exactly:

• POINTER-tagged heap handle (a claimed receiver may be a String or a number → call);
• GC_TYPE_ARRAY, not GC_FLAG_FORWARDED, none of FROZEN|SEALED|NO_EXTEND|ARRAY_DESCRIPTORS (0x407) in _reserved (frozen / non-writable-length arrays must throw, descriptor-bearing ones need the descriptor-aware [[Delete]]);
• PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED == 0 (polluted Array.prototype);
• 0 < length <= capacity <= 1e8 and the popped slot is not the hole sentinel.

The inline arm reads the element and stores the decremented i32 length — no pointer store, no allocation, no GC bookkeeping, which is also all the runtime's fast path does. The empty array still calls (that arm is #8934's runtime fast path to keep). No runtime changes.

Why

A fresh symbol-level profile of the wolf-ecs twins (Linux perf, 10 s): js_array_pop_f64 is 10.1% of self time in the entity cycle and 8.1% in add/remove, while every pop there (SparseSet.remove's and createEntity's packed.pop()) takes the runtime's plain-array fast path — the cost is the call, the heap-address classification (try_read_gc_header) and the flag resolution around a single length store.

Verification (local, per the campaign rule; no CI wait)

• IR census test array_pop_takes_the_inline_tier_with_the_runtime_call_as_fallback for both routes: apop.hdr tests GC_TYPE_ARRAY, the 1031 mask and the latch; apop.read tests the hole sentinel; apop.take is one store i32 and no call; apop.slow calls js_array_pop_f64.
• Gate run (suites, -D warnings, file size, GC store inventory, raw-handle debt, census, local-binding, addr-class) and an e2e semantics test (holes, frozen, non-writable length, Array subclass, string receiver, accessor slot, polluted prototype — node-identical output) are in progress; Mac-mini paired screens main → PR (2 s + 50 ms, 11 pairs) follow. I'll post all three as comments.

https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ

Summary by CodeRabbit

• Performance

  • Improved Array.prototype.pop() performance for eligible plain arrays by handling common cases inline.
  • Preserved the existing fallback behavior for arrays and conditions that cannot use the optimized path.

• Bug Fixes

  • Ensured empty arrays return undefined correctly.
  • Added safeguards for unsupported array states, sparse arrays, and invalid lengths.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ce33cfcc-5ede-4974-8d51-9cc89ab344f2

📥 Commits

Reviewing files that changed from the base of the PR and between a1e44c5 and 8f9614a.

📒 Files selected for processing (5)

• changelog.d/0000-inline-array-pop-tier.md
• crates/perry-codegen/src/expr/array_pop.rs
• crates/perry-codegen/src/expr/mod.rs
• crates/perry-codegen/src/lower_array_method.rs
• crates/perry-codegen/src/lower_call/native/native_instance_branch.rs

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

---

📝 Walkthrough

Walkthrough

arr.pop() now uses an inline plain-array lowering for Array-admitted receivers. The lowering checks heap metadata, length, capacity, prototype state, and holes. Eligible arrays update length inline; other cases call js_array_pop_f64.

Changes

Inline Array Pop

Layer / File(s)	Summary
Inline pop lowering and validation crates/perry-codegen/src/expr/array_pop.rs, crates/perry-codegen/src/expr/mod.rs, changelog.d/0000-inline-array-pop-tier.md	Adds lower_array_pop_inline with pointer, heap, GC header, flag, prototype-pollution, length, capacity, and hole checks. Eligible arrays return the final element after one length store. Other cases use js_array_pop_f64. Tests cover both receiver routes and the fallback.
Array pop call-site integration crates/perry-codegen/src/lower_array_method.rs, crates/perry-codegen/src/lower_call/native/native_instance_branch.rs	Routes claimed-array and native array.pop or array.pop_back lowering through lower_array_pop_inline. The helper returns a boxed value, including boxed undefined for an empty array.

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

Merge Risk: 🔵 Low · up to 8f961

The optimization adds a direct array read and length write for eligible receivers; if a non-array heap address is misclassified, it could read or modify invalid object layout. The existing fallback limits impact, but the admission logic should be aligned with the canonical classifier or the exclusion explicitly validated before merge.

Sequence Diagram(s)

sequenceDiagram
  participant ArrayPopCaller
  participant lower_array_pop_inline
  participant GCHeap
  participant js_array_pop_f64

  ArrayPopCaller->>lower_array_pop_inline: pass boxed array receiver
  lower_array_pop_inline->>GCHeap: validate array metadata and read final element
  alt inline checks pass
    lower_array_pop_inline->>GCHeap: decrement and store length
    lower_array_pop_inline-->>ArrayPopCaller: return boxed popped element
  else fallback condition
    lower_array_pop_inline->>js_array_pop_f64: execute runtime pop
    js_array_pop_f64-->>ArrayPopCaller: return runtime result
  end

Loading

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the implementation, motivation, scope, and verification status, but it does not follow the repository template. It omits the required Summary, Changes, Related issue, Test pla…	Rewrite the description using the repository template. Add the required section headings, list concrete changes, state the related issue or use "n/a", provide completed test commands and results, complete the checklist, and include output i…
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: inlining the plain-array pop tier while retaining js_array_pop_f64 as the fallback.
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.

Full details: Description check

Explanation

The description explains the implementation, motivation, scope, and verification status, but it does not follow the repository template. It omits the required Summary, Changes, Related issue, Test plan, Screenshots / output, and Checklist sections, and it reports key verification work as still in progress.

Resolution

Rewrite the description using the repository template. Add the required section headings, list concrete changes, state the related issue or use "n/a", provide completed test commands and results, complete the checklist, and include output if applicable.

Full details: Docstring Coverage

Explanation

Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[proggeramlug]

Merged. The inline admission mirrors the runtime fast path's decision — POINTER-tagged heap handle, GC_TYPE_ARRAY, not GC_FLAG_FORWARDED, and none of FROZEN|SEALED|NO_EXTEND|ARRAY_DESCRIPTORS (0x407) — so frozen and descriptor-bearing arrays keep the runtime path that can throw or run the descriptor-aware [[Delete]]. That gating is the part that had to be exact, and it is.

Validation: codegen 1334/0, runtime 2771/0 (RUST_TEST_THREADS=1); scripts/run_lint_gates.sh 57 of 58 including the compile tier green (-D warnings and clippy), the exception being the pre-existing ${{ }} substitution artifact (#8929).

[proggeramlug]

Gates on 8f9614a8f (Linux host): RUSTFLAGS=-D warnings cargo check --workspace --all-targets clean; cargo test -p perry-codegen -p perry-transform -p perry-hir 2505 passed / 0 failed; file size OK; GC store-site inventory passed; raw-handle debt unchanged (967, none raised); shape census OK; local-binding audit OK; addr-class audit passed.

Semantics probe (holes, frozen, non-writable length, Array subclass, accessor slot, polluted Array.prototype, empty): identical before and after the tier. Two pre-existing gaps surfaced by the probe, unchanged by this PR (both route to the runtime call exactly as before): popping a hole yields NaN where Node yields undefined, and "abc".pop() through an erased field does not throw. Noting them separately; the e2e test in this PR pins the cases where perry already matches Node.

Mac-mini paired screens (11 pairs), main a1e44c5df → this PR:

window	add/remove	entity cycle
2 s	0.4031 → 0.4040 (+0.20%, 0/11)	0.3348 → 0.3341 (−0.23%, 11/11)
50 ms	0.4030 → 0.4039 (+0.25%, 2/11)	0.3351 → 0.3343 (−0.28%, 11/11)

That is far less than 8–10% of pop self time should buy, and add/remove moves the wrong way, so I'm not calling this a retain yet — re-profiling the PR build on Linux to see whether the inline arm is actually being taken on the hot pops (if js_array_pop_f64 self time is gone and the residual is code-size cost in the hot bodies, this PR gets closed like #8940).

[proggeramlug]

Resolved why the ECS numbers are small: wolf-ecs's Archetype constructor does this.sset.packed = this, so the hot sset.packed.pop() / .push() / [i] in add/remove/destroy run on the object-backed Array subclass itself, not on a plain array (gdb at the residual js_array_pop_f64 call from destroyEntity$pshape: receiver obj_type=2, _reserved=0x800, latch clear). The tier correctly declines those (obj_type ≠ ARRAY) — that is the 8–10% "pop" self time, and it is next-list item #3 (object-backed push/pop), not plain-array pop. The only plain pops in the cycle are _rm.packed.pop() in createEntity, which is the consistent −0.25% (11/11) on the entity cycle; add/remove has no plain pops, so its +0.2% (0/11) is the cost of the ~8-instruction pre-check in front of the runtime call on object-backed receivers.

Where the tier does apply it pays: plain-array push/pop microbenchmark (a class field packed = [], 20 000 rounds of 500 pop + 500 push, Linux x86_64, 5 alternating runs): main-equivalent 133.6 / 133.9 / 134.1 / 134.0 / 136.7 ms → this PR 123.5 / 128.2 / 123.1 / 123.0 / 125.7 ms (−7.7% median), with pushes (and their GC bookkeeping) still in the loop — so per pop it is well over that.

Recommendation: merge as a general improvement (semantics pinned by the e2e test, f87f43b5a; all gates green); it is not an ECS-campaign retain by the 11/11-both rule, and I've said so above. The object-backed subclass push/pop path is the actual ECS cost center and is what I'm looking at next.