perf: three fixed per-call costs — concat-chain stack scratch, the Any/Any === tail, and the opaque write barrier - #7885
Merged
Merged
Conversation
added 4 commits
August 11, 2026 21:05
Every call memset ~2 KB of stack regardless of part count.
📝 WalkthroughWalkthroughThe PR adds inline strict-equality lowering, conditional write-barrier emission, and size-specialized string-concatenation scratch buffers. It also adds an INT32 tag constant, lazy numeric buffer initialization, benchmark measurements, and a changelog. ChangesPer-call cost reductions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GenericEquality
participant InlineLowering as lower_strict_eq_inline_any
participant RuntimeEquality as js_eq
GenericEquality->>InlineLowering: Pass tagged operands
InlineLowering-->>GenericEquality: Return inline boolean for decidable values
InlineLowering->>RuntimeEquality: Compare unsupported or unsafe values
RuntimeEquality-->>GenericEquality: Return equality result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
changelog.d/7885-fixed-per-call-costs.md (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep internal benchmark methodology out of the release fragment.
The host, load, foreign-process, emitted-IR, binary-identity, and noise-floor details describe one development run. Keep concise probe results in the changelog. Move measurement methodology to PR notes or benchmark documentation.
Based on learnings: “For PerryTS/perry changelog fragments under
changelog.d/, describe the final shipped behavior as one coherent release-note entry. Do not include separate development-slice narratives that may contradict one another when the release notes are assembled.”Also applies to: 49-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelog.d/7885-fixed-per-call-costs.md` around lines 34 - 39, Trim the changelog fragment around the benchmark results to a concise release-note entry describing the shipped behavior and probe outcomes. Remove internal measurement methodology and development-run details such as host, load, process, IR, binary-identity, and noise-floor information; retain only the relevant concise benchmark results, and present them as one coherent entry.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7885-fixed-per-call-costs.md`:
- Around line 40-47: Update the benchmark table header to include a change
column, then make the final “other 18 corpus programs” value use the same
percentage-change format as the preceding rows, or explicitly label that column
as an after/before ratio and clarify the preceding percentage values
accordingly. Ensure every row’s metric has a consistent, interpretable unit.
In `@crates/perry-codegen/src/expr/compare.rs`:
- Around line 383-407: The address-classification literals must stay
synchronized with the runtime contract before the forwarding block dereferences
addr - 7. In crates/perry-codegen/src/expr/compare.rs lines 383-407, verify or
adjust HANDLE_BAND_MAX_I64 and HEAP_ADDR_CEILING_I64 so the magnitude window
admits no address rejected by perry-runtime::value::addr_class/is_valid_obj_ptr;
in crates/perry-codegen/src/expr/compare.rs lines 259-267, add a test that
parses both literals and asserts they equal the runtime constants, following
tag_strings_match_u64_values.
In `@crates/perry-runtime/src/string/concat.rs`:
- Around line 479-480: Update the count handling in the concat function to
reject non-positive i32 values before converting n to usize, preventing negative
counts from being clamped and used as 32 parts. Preserve the existing zero-count
or null-parts early return and only apply CONCAT_CHAIN_MAX_PARTS after n is
known to be positive.
- Around line 479-512: Update js_string_concat_chain to return the empty string
when n <= 0 or parts is null before casting n to usize; only cast positive
values, then retain the existing CONCAT_CHAIN_MAX_PARTS clamp and sized
dispatch. Afterward, run the required perry-dev checks, rebuild both static
wrapper crates, and execute runtime tests with RUST_TEST_THREADS=1.
- Around line 479-496: Update drain_whole_buffer’s handling of values passed to
js_string_concat_chain so inputs exceeding CONCAT_CHAIN_MAX_PARTS are not
truncated by the runtime clamp. Add a non-truncating batching or equivalent
concatenation fallback for oversized decoded buffers, while preserving the
existing direct path for chains within the 32-part limit.
---
Nitpick comments:
In `@changelog.d/7885-fixed-per-call-costs.md`:
- Around line 34-39: Trim the changelog fragment around the benchmark results to
a concise release-note entry describing the shipped behavior and probe outcomes.
Remove internal measurement methodology and development-run details such as
host, load, process, IR, binary-identity, and noise-floor information; retain
only the relevant concise benchmark results, and present them as one coherent
entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 032d2596-4ca8-4b6b-91a5-0f7d0375bc18
📒 Files selected for processing (5)
changelog.d/7885-fixed-per-call-costs.mdcrates/perry-codegen/src/expr/compare.rscrates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/nanbox.rscrates/perry-runtime/src/string/concat.rs
Comment on lines
+40
to
+47
| | bench | before | after | | | ||
| |---|--:|--:|--:| | ||
| | `strbuild` (concat probe) | 0.7448 | 0.5477 | −26.5% | | ||
| | `eqscan` (key-scan probe) | 0.2181 | 0.1720 | −21.1% | | ||
| | `iso_miss` | 1.2319 | 1.1283 | −8.4% | | ||
| | `pipeline_big` | 2.5334 | 2.3291 | −8.1% | | ||
| | `pipeline` | 0.2646 | 0.2444 | −7.6% | | ||
| | the other 18 corpus programs | — | — | 0.978 – 1.005 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label and normalize the benchmark metrics.
The final table column has no heading. The first rows use percentage changes, but the final row uses 0.978 – 1.005 without defining its unit. Readers cannot interpret the last row consistently.
Add a change heading. If the range is an after/before ratio, convert it to percentages or label the ratio explicitly.
Proposed table clarification
-| bench | before | after | |
+| bench | before (s) | after (s) | change |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7885-fixed-per-call-costs.md` around lines 40 - 47, Update the
benchmark table header to include a change column, then make the final “other 18
corpus programs” value use the same percentage-change format as the preceding
rows, or explicitly label that column as an after/before ratio and clarify the
preceding percentage values accordingly. Ensure every row’s metric has a
consistent, interpretable unit.
Comment on lines
+383
to
+407
| // Both POINTER_TAG. Classify by magnitude before touching a header. | ||
| ctx.current_block = band_idx; | ||
| let l_addr = ctx.block().and(I64, &l_bits, POINTER_MASK_I64); | ||
| let r_addr = ctx.block().and(I64, &r_bits, POINTER_MASK_I64); | ||
| let l_above = ctx.block().icmp_uge(I64, &l_addr, HANDLE_BAND_MAX_I64); | ||
| let l_below = ctx.block().icmp_ult(I64, &l_addr, HEAP_ADDR_CEILING_I64); | ||
| let r_above = ctx.block().icmp_uge(I64, &r_addr, HANDLE_BAND_MAX_I64); | ||
| let r_below = ctx.block().icmp_ult(I64, &r_addr, HEAP_ADDR_CEILING_I64); | ||
| let l_heap = ctx.block().and(I1, &l_above, &l_below); | ||
| let r_heap = ctx.block().and(I1, &r_above, &r_below); | ||
| let both_heap = ctx.block().and(I1, &l_heap, &r_heap); | ||
| ctx.block().cond_br(&both_heap, &fwd_l, &slow_l); | ||
|
|
||
| ctx.current_block = fwd_idx; | ||
| let l_flags_addr = ctx.block().sub(I64, &l_addr, "7"); | ||
| let l_flags_ptr = ctx.block().inttoptr(I64, &l_flags_addr); | ||
| let l_flags = ctx.block().load(I8, &l_flags_ptr); | ||
| let r_flags_addr = ctx.block().sub(I64, &r_addr, "7"); | ||
| let r_flags_ptr = ctx.block().inttoptr(I64, &r_flags_addr); | ||
| let r_flags = ctx.block().load(I8, &r_flags_ptr); | ||
| let either = ctx.block().or(I8, &l_flags, &r_flags); | ||
| // GC_FLAG_FORWARDED = 0x80; LLVM i8 literals are signed. | ||
| let fwd_bits = ctx.block().and(I8, &either, "-128"); | ||
| let no_fwd = ctx.block().icmp_eq(I8, &fwd_bits, "0"); | ||
| ctx.block().cond_br(&no_fwd, &false_l, &slow_l); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The codegen copy of the runtime address-classification contract is unverified. Both sites depend on HANDLE_BAND_MAX_I64 and HEAP_ADDR_CEILING_I64 matching perry-runtime::value::addr_class, and nothing binds them. A divergence turns a js_eq call into a load from unmapped memory.
crates/perry-codegen/src/expr/compare.rs#L383-L407: confirm the magnitude window admits no address thatis_valid_obj_ptrrejects, before thefwdblock loadsaddr - 7.crates/perry-codegen/src/expr/compare.rs#L259-L267: add a test that parses both literals and asserts equality with the runtime constants, in the style oftag_strings_match_u64_valuesincrates/perry-codegen/src/nanbox.rs.
📍 Affects 1 file
crates/perry-codegen/src/expr/compare.rs#L383-L407(this comment)crates/perry-codegen/src/expr/compare.rs#L259-L267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/expr/compare.rs` around lines 383 - 407, The
address-classification literals must stay synchronized with the runtime contract
before the forwarding block dereferences addr - 7. In
crates/perry-codegen/src/expr/compare.rs lines 383-407, verify or adjust
HANDLE_BAND_MAX_I64 and HEAP_ADDR_CEILING_I64 so the magnitude window admits no
address rejected by perry-runtime::value::addr_class/is_valid_obj_ptr; in
crates/perry-codegen/src/expr/compare.rs lines 259-267, add a test that parses
both literals and asserts they equal the runtime constants, following
tag_strings_match_u64_values.
Comment on lines
+479
to
+480
| let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS); | ||
| if n == 0 || parts.is_null() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Reject negative n before the cast.
n is an i32. A negative value wraps to a large usize, and .min(CONCAT_CHAIN_MAX_PARTS) changes it to 32. With a non-null parts, the function then reads 32 elements for a negative count.
Check n <= 0 before converting it, or use usize::try_from(n).
Proposed fix
- let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS);
- if n == 0 || parts.is_null() {
+ if n <= 0 || parts.is_null() {
return crate::string::js_string_from_bytes(b"".as_ptr(), 0);
}
+ let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS); | |
| if n == 0 || parts.is_null() { | |
| if n <= 0 || parts.is_null() { | |
| return crate::string::js_string_from_bytes(b"".as_ptr(), 0); | |
| } | |
| let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/string/concat.rs` around lines 479 - 480, Update the
count handling in the concat function to reject non-positive i32 values before
converting n to usize, preventing negative counts from being clamped and used as
32 parts. Preserve the existing zero-count or null-parts early return and only
apply CONCAT_CHAIN_MAX_PARTS after n is known to be positive.
Comment on lines
+479
to
+496
| let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS); | ||
| if n == 0 || parts.is_null() { | ||
| return crate::string::js_string_from_bytes(b"".as_ptr(), 0); | ||
| } | ||
| if parts.is_null() { | ||
| return crate::string::js_string_from_bytes(b"".as_ptr(), 0); | ||
|
|
||
| // ★ Size the stack scratch to the chain actually being built. One | ||
| // `MAX_PARTS = 32` shape made EVERY call pay ~2 KB of stack | ||
| // initialisation — the release disassembly opens `sub sp, sp, #0x7e0`, | ||
| // then `memset(sp+0x20, _, 0x400)` for `num_bufs`, then 32 `str xzr` for | ||
| // the handle array — whether the chain had 32 parts or 2. Real chains are | ||
| // 2-4 parts: `seen = seen + "[" + names[i] + "]"` in an environment-lookup | ||
| // loop is four, and was memsetting 2 KB per append. | ||
| if n <= 4 { | ||
| concat_chain_sized::<4>(parts, n) | ||
| } else if n <= 8 { | ||
| concat_chain_sized::<8>(parts, n) | ||
| } else { | ||
| concat_chain_sized::<CONCAT_CHAIN_MAX_PARTS>(parts, n) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="crates/perry-codegen/src/lower_string_concat.rs"
ast-grep outline "$file" --items all --type function
rg -n -C 12 \
'CONCAT_CHAIN_MAX_PARTS|alloca_entry_array|lowered\.len\(\)|lowered\.iter\(\)\.enumerate\(\)|js_string_concat_chain' \
"$file"Repository: PerryTS/perry
Length of output: 5785
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- codegen callers and flattening ---'
sed -n '560,690p' crates/perry-codegen/src/lower_string_concat.rs
rg -n -C 20 'flatten_string_add_chain|lower_string_concat_chain|CONCAT_CHAIN_MAX_PARTS' crates/perry-codegen crates/perry-runtime
printf '%s\n' '--- runtime entry and helper ---'
sed -n '430,530p' crates/perry-runtime/src/string/concat.rs
sed -n '530,680p' crates/perry-runtime/src/string/concat.rsRepository: PerryTS/perry
Length of output: 44321
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("crates/perry-codegen/src/lower_string_concat.rs")
s = p.read_text()
start = s.index("pub(crate) fn flatten_string_add_chain")
end = s.index("pub(crate) fn lower_string_concat_chain", start)
print(s[start:end])
PY
rg -n -C 25 'flatten_string_add_chain\(' crates/perry-codegenRepository: PerryTS/perry
Length of output: 8033
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all direct references ---'
rg -n 'js_string_concat_chain|lower_string_concat_chain\(' . \
-g '!target/**' -g '!node_modules/**'
printf '%s\n' '--- count-bound model for flatten_string_add_chain ---'
python3 - <<'PY'
MAX = 32
max_returned = 0
returning_cases = []
for accepted_inner_adds in range(0, 100):
count = 1 # initial right operand
rejected = False
for _ in range(accepted_inner_adds):
count += 1 # push the accepted Add's right operand
if count >= MAX:
rejected = True
break
if rejected:
continue
count += 1 # terminal leaf or opaque subtree
max_returned = max(max_returned, count)
returning_cases.append((accepted_inner_adds, count))
print("max returned parts:", max_returned)
print("first rejected count:", next(
(accepted_inner_adds + 2 for accepted_inner_adds in range(100)
if 1 + (accepted_inner_adds + 1) >= MAX),
None,
))
print("returned cases over MAX:", [
case for case in returning_cases if case[1] > MAX
][:5])
PYRepository: PerryTS/perry
Length of output: 9989
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '70,135p' crates/perry-runtime/src/node_stream_readable_read.rs
rg -n -C 20 'values\.push|values\s*=|js_string_concat_chain' crates/perry-runtime/src/node_stream_readable_read.rs crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,145p' crates/perry-runtime/src/node_stream_readable_read.rs
rg -n -C 12 'js_string_concat_chain|let mut values|values\.push|values\.len' crates/perry-runtime/src/node_stream_readable_read.rsRepository: PerryTS/perry
Length of output: 10570
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 20 '^fn push_chunk_values|push_chunk_values\(' \
crates/perry-runtime/src/node_stream_readwrite.rs \
crates/perry-runtime/src/node_stream_pipeline.rs \
crates/perry-runtime/src/node_stream_readable_read.rs
rg -n -C 8 'drain_whole_buffer|read_stream_available_default|more than 32|32.*chunk|chunk.*32' \
crates/perry-runtime tests test-filesRepository: PerryTS/perry
Length of output: 49288
Handle oversized decoded stream buffers without truncation.
The codegen path enforces the 32-part bound, but drain_whole_buffer passes all decoded chunks to js_string_concat_chain. When values.len() > 32, the runtime clamp drops the suffix. Use a non-truncating fallback, such as batched concatenation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/string/concat.rs` around lines 479 - 496, Update
drain_whole_buffer’s handling of values passed to js_string_concat_chain so
inputs exceeding CONCAT_CHAIN_MAX_PARTS are not truncated by the runtime clamp.
Add a non-truncating batching or equivalent concatenation fallback for oversized
decoded buffers, while preserving the existing direct path for chains within the
32-part limit.
Comment on lines
+479
to
+512
| let n = (n as usize).min(CONCAT_CHAIN_MAX_PARTS); | ||
| if n == 0 || parts.is_null() { | ||
| return crate::string::js_string_from_bytes(b"".as_ptr(), 0); | ||
| } | ||
| if parts.is_null() { | ||
| return crate::string::js_string_from_bytes(b"".as_ptr(), 0); | ||
|
|
||
| // ★ Size the stack scratch to the chain actually being built. One | ||
| // `MAX_PARTS = 32` shape made EVERY call pay ~2 KB of stack | ||
| // initialisation — the release disassembly opens `sub sp, sp, #0x7e0`, | ||
| // then `memset(sp+0x20, _, 0x400)` for `num_bufs`, then 32 `str xzr` for | ||
| // the handle array — whether the chain had 32 parts or 2. Real chains are | ||
| // 2-4 parts: `seen = seen + "[" + names[i] + "]"` in an environment-lookup | ||
| // loop is four, and was memsetting 2 KB per append. | ||
| if n <= 4 { | ||
| concat_chain_sized::<4>(parts, n) | ||
| } else if n <= 8 { | ||
| concat_chain_sized::<8>(parts, n) | ||
| } else { | ||
| concat_chain_sized::<CONCAT_CHAIN_MAX_PARTS>(parts, n) | ||
| } | ||
| } | ||
|
|
||
| /// The body of [`js_string_concat_chain`], monomorphised on the scratch-array | ||
| /// size. `0 < n <= MAX_PARTS` and `!parts.is_null()` are preconditions the | ||
| /// dispatcher establishes. | ||
| fn concat_chain_sized<const MAX_PARTS: usize>(parts: *const f64, n: usize) -> *mut StringHeader { | ||
| debug_assert!(n > 0 && n <= MAX_PARTS); | ||
| // Per-part scratch buffer for number formatting. 32 bytes is enough | ||
| // for any f64 string representation (max ~24 chars). | ||
| let mut num_bufs: [[u8; 32]; MAX_PARTS] = [[0u8; 32]; MAX_PARTS]; | ||
| // for any f64 string representation (max ~24 chars). Left UNINITIALISED: | ||
| // a slot becomes readable only via `MaybeUninit::write`, on exactly the | ||
| // two numeric arms, which are also the only arms that publish a | ||
| // `piece_ptrs[i]` into it — so the copy loop can never read an | ||
| // uninitialised slot. | ||
| let mut num_bufs: [core::mem::MaybeUninit<[u8; 32]>; MAX_PARTS] = | ||
| [core::mem::MaybeUninit::uninit(); MAX_PARTS]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
cargo check -p perry --profile perry-dev
cargo build -p perry-runtime-static --profile perry-dev
cargo build -p perry-stdlib-static --profile perry-dev
RUST_TEST_THREADS=1 cargo test -p perry-runtime --profile perry-devRepository: PerryTS/perry
Length of output: 1139
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- concat implementation ---'
sed -n '430,630p' crates/perry-runtime/src/string/concat.rs
printf '%s\n' '--- concat-chain call sites and limits ---'
rg -n -C 4 'js_string_concat_chain|CONCAT_CHAIN_MAX_PARTS|lowered\.len|concat_chain' crates
printf '%s\n' '--- workspace package/profile metadata ---'
rg -n -C 3 'perry-dev|perry-runtime-static|perry-stdlib-static|name = "perry"' Cargo.toml crates/*/Cargo.tomlRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- flatten and lowering implementation ---'
sed -n '560,675p' crates/perry-codegen/src/lower_string_concat.rs
printf '%s\n' '--- workspace profiles ---'
rg -n -C 5 '^\[profile\.perry-dev\]|perry-dev|^\[workspace\]' Cargo.toml crates/*/Cargo.toml
printf '%s\n' '--- static wrapper manifests ---'
cat crates/perry-runtime-static/Cargo.toml
cat crates/perry-stdlib-static/Cargo.toml
printf '%s\n' '--- signed-count behavior ---'
python3 - <<'PY'
for n in (-2, -1, 0, 1, 32, 33):
converted = n % (1 << 64)
print(f"n={n:>3} -> usize={converted} -> min(32)={min(converted, 32)}")
PYRepository: PerryTS/perry
Length of output: 8532
Reject negative n values before casting
js_string_concat_chain casts n: i32 to usize before validation. A negative ABI input becomes usize::MAX, is clamped to 32, and causes the function to read 32 values. Handle n <= 0 before the cast.
Run the required checks with the perry-dev profile, rebuild both static wrapper crates, and run runtime tests with RUST_TEST_THREADS=1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/string/concat.rs` around lines 479 - 512, Update
js_string_concat_chain to return the empty string when n <= 0 or parts is null
before casting n to usize; only cast positive values, then retain the existing
CONCAT_CHAIN_MAX_PARTS clamp and sized dispatch. Afterward, run the required
perry-dev checks, rebuild both static wrapper crates, and execute runtime tests
with RUST_TEST_THREADS=1.
Source: Coding guidelines
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
1 participant
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.
Three fixed per-call costs found by reading the shipped disassembly and the emitted
IR of
gc-handoff/apps/iso_miss.tsandgc-handoff/apps/pipeline.ts. Each isindependent, each is committed separately, and each has its own probe.
1.
js_string_concat_chaininitialised ~2 KB of stack on every callThe helper sized its scratch at the codegen cap of 32 parts:
so a two-part chain paid exactly what a 32-part chain paid. Read out of the shipped
libperry_runtime.a(ar xthe cgu,otool -tv), not inferred from the source:Real chains are 2-4 parts.
iso_miss's environment lookup runsseen = seen + "[" + names[i] + "]"on every name comparison, and the codegenN-ary fold turns each of those into exactly one call — so each append memset 2 KB.
★ Worth stating because I expected the opposite: the N-ary fold is not missing. It
fires (verified in
--trace llvm). The cost was inside the helper it calls, and it isindependent of the string lengths, which is why it bites a program whose accumulator
never exceeds ~20 bytes.
The body is now monomorphised on the scratch size (
n<=4/n<=8/<=32) withnum_bufsleftMaybeUninit, so only slots a numeric arm actually formats into arewritten. Post-fix disassembly of the
n<=4arm has nomemsetand fourstr xzr.2. A strict
===with two statically-unconstrained operands was onejs_eqcallThe shape that pays is a linear scan over a generic container's key array —
this.keys[i] === kin aRegistry<K, V>. Verified inpipeline's IR:call i64 @js_eqappears exactly inRegistry$fn_num'sset/getand not inRegistry$str_num's, which take the string arms.A scan is dominated by misses (a hit ends the loop), and a miss is the expensive
direction: it reaches
js_jsvalue_equals's pointer arm, which runsresolve_forwardingtwice — band classification, small-buffer-slab probe, header read, per operand. A
hit-only fast path would have measured nothing here.
The new inline prefix settles four cases without a call, each an exact restatement of
what
js_jsvalue_equalscomputes rather than an approximation:occupy top16
0x7FF9..=0x7FFF, so a tagged immediate stays equal to itself while0x7FF8…/0xFFF8…NaN falls out to the call, which answersfalse;POINTER_TAG, distinct in-band addresses, and neitherGcHeadercarriesGC_FLAG_FORWARDED⇒ distinct objects. This isresolve_forwarding's own "neither isforwarded, so fall through to 0"; anything forwarded (a post-
js_array_growalias, astale pre-evacuation pointer) still takes the call. The header read is the one
expr/array_push.rsalready emits, behind the magnitude guard the runtime appliesbefore any dereference.
3. The opaque write-barrier wrapper had no value test
write_barrier_slot_inner's first action isbarrier_child_prologue(child)?, sojs_write_barrierdoes nothing at all for a non-pointer child. But an array elementstore on a
number[]emitted a bare unconditional call next to every element write(
this.vals[i] = vin theRegistryabove). #7511 put exactly this gate on theclass-field slot store;
emit_write_barriernever got it. It now sits behindemit_may_carry_heap_pointer_check, a deliberate superset of the runtime predicate(
gc::tests::inline_pointer_bearing_contractenumerates the whole 16-bit tag spaceagainst it), so it can only skip calls that would have returned immediately.
Probes
gc-handoff/bench/strbuild.ts— 10M four-part concat chains (node:40000000).gc-handoff/bench/eqscan.ts— 2.4M generic-container key scans (node:2400000 3).Measured — quiet M1 mini, best-of-5, interleaved, exit-checked
LOAD_BEFORE=1.79 LOAD_AFTER=2.04 PROC_AFTER=0→ VERDICT: CLEAN. Every cell exit 0.Arms are cumulative; the two codegen arms share one pinned runtime archive, so
cmpis meaningful across them.
===)Each probe moves several times more than the application it was extracted from, which is
the evidence that it exercises its subject rather than something adjacent.
Validation
node --experimental-strip-types, exit 0 (incl. theiso_misscanarychecksum 437840 misses 0)cmpA vs B, one runtime pinned, basenames constantAny/Any===cmpB vs C, sameemit_write_barriersitejs_string_concat_chaincall sites in their emitted IR — the runtime change cannot reach them, and they measured 0.978–1.005otool -tvof the archive'sn<=4arm has nomemset;--trace llvmshows 72anyeq.*blocks and 6wb.maybeblocks inpipelinePERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_FORCE_EVACUATE=1+ PERRY_GC_PROTECT_FROMSPACE=1 DEPTH=800 SCHEDULE_RATE=1cargo test --release -p perry-codegen --libRUST_TEST_THREADS=1 cargo test --release -p perry-runtime --lib9ca8b4f71(5CRASHin the http/net/fetch family, 6PARITY_FAIL), so none is caused by this branch. A further 10 movednode_fail -> parity_fail, i.e. the local oracle differs from the one that took the snapshot.cargo fmt --all -- --check,check_file_size.sh,addr_class_inventory.py,gc_runtime_root_holders.py