diff --git a/CLAUDE.md b/CLAUDE.md index 56e292a1e4..e92697aa20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Perry is a native TypeScript compiler written in Rust that compiles TypeScript source code directly to native executables. It uses SWC for TypeScript parsing and LLVM for code generation. -**Current Version:** 0.5.1331 +**Current Version:** 0.5.1332 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index f911f0e619..e62a263efc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1331" +version = "0.5.1332" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1331" +version = "0.5.1332" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1331" +version = "0.5.1332" [[package]] name = "perry-ui-tvos" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1331" +version = "0.5.1332" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index a7070c836e..b5f7fbf958 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1331" +version = "0.5.1332" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7586-typed-shape-install-constant-folding.md b/changelog.d/7586-typed-shape-install-constant-folding.md new file mode 100644 index 0000000000..295ca6cdb3 --- /dev/null +++ b/changelog.d/7586-typed-shape-install-constant-folding.md @@ -0,0 +1,157 @@ +**The typed-shape install stops re-deriving compile-time mask facts on every +construction. `push_cls` 1.09x, `churn_alloc` 1.07x, `churn` 1.04x, at +0 bytes +(#7578).** + +`gc::layout::typed_shape_layout_entry` was re-measured on the pinned quiet host +before any code was written, because three tickets in this campaign were worked +from a headline that had already collapsed. It had not: **22.7% of `push_cls` +self time**, the largest single symbol in object construction, with +`layout_forget_object` a further 4.1% — 26.7% between them for a steady-state +path whose entire content is two header bit writes. + +### The ticket's stated hypothesis was wrong, and the disassembly says why + +#7578 proposed that the cost is the FFI call and the thread-local resolutions it +performs — the shape #7566 won 1.81x on — and that the remedy is to emit the hit +path inline at the `new` site. The emitted prologue looked like it agreed: + +``` +sub sp, sp, #0x150 ; a 336-byte frame +stp x28, x27, [sp, #0xf0] ; ...and six pairs, twelve callee-saved registers +stp x26, x25, [sp, #0x100] ; spilled and reloaded on every construction +... +``` + +LLVM sizes a prologue for a function's worst path, and the worst path here — the +descriptor build, the `RefCell` borrow on `SHAPE_LAYOUTS`, the hash — runs once +per shape. **That hypothesis was tested and is false.** Outlining the install +behind `#[cold] #[inline(never)]` cut the frame to 80 bytes and the twelve +callee-saved spills to zero, and made the benchmark *slower*: `push_cls` 0.72 -> +0.75 s, `churn_alloc` 0.72 -> 0.79 s, reproduced across two runs. On this core +the spills are cheap dual-issued stores off the critical path; removing them +bought nothing, and keeping six arguments live to forward to the outlined call +cost a dozen register moves that were not there before. **Instruction count is +what this function is bound by, not frame size** — which is also why #7566's +inline-bump win does not generalise here. + +### Where the 22.7% actually goes + +Counted off the disassembly, roughly 30 of the ~70 instructions on the hit path +re-derive facts that are **compile-time constants of the class**, per call, +because the FFI boundary makes them opaque parameters: + +- **12 instructions** normalising two `(pointer, length)` pairs into slices. + `slice::from_raw_parts` needs a non-null aligned pointer, so a `null` mask + became `NonNull::dangling()` through a `csel` chain — to build two slices the + hit path then only ever compares as integers. +- **~11 instructions** of `words_intersect` setup, asking whether the raw-f64 + and pointer masks overlap. They are two immutable globals; the answer was + fixed when the class was compiled. +- **~6 instructions** computing `gc_type_layout_slot_kind` a second time. + `layout_header_for_user` already computed the kind and accepted three of them; + the next line loaded through the 32-byte-strided type table again to narrow + those three to one. + +So the fix is not to move the call, it is to stop paying for the constants. + +- The construction path now carries the raw `(pointer, word count)` pairs and + materialises a slice only where one is actually indexed — the validating entry + point's per-slot loop, and the install. +- The mask-disjointness check moves **below** the memo probe. A hit proves an + install already ran it over the *same* globals and passed: a shape whose masks + intersect is downgraded before it can reach `record`, so no intersecting tuple + can be in the table to hit. It still runs ahead of every install, which is the + only place its answer is used. +- The memo carries one bit, `dims` bit 62: whether the pointer mask is empty, + which selects `GC_LAYOUT_POINTER_FREE` vs `GC_LAYOUT_SIDE_MASK`. +- One `gc_type_layout_slot_kind`, not two. + +**Why replaying two mask predicates is not the thing the memo's soundness bar +forbids.** That bar is about the *object*: `field_count == slot_count` and the +per-slot validation stay per-instance, because an object's contents change under +the mutator. These two predicates read only the mask globals, which are +codegen-emitted `private unnamed_addr constant`s — in the read-only image, never +written, never freed. An entry matches on those globals' addresses and lengths, +so a matching address *is* a matching byte string for the life of the process, +and a pure function of those bytes has one answer forever. The residual failure +mode is still a miss, never a wrong hit. + +| bench | main | this | ratio | +|---|--:|--:|--:| +| `push_cls` | 0.72 s | **0.66 s** | **1.091x** | +| `churn_alloc` | 0.72 s | **0.67 s** | **1.075x** | +| `churn` | 1.00 s | **0.96 s** | **1.042x** | +| `cycles` | 0.83 s | 0.83 s | 1.00x | +| `retain` | 2.89 s | 2.88 s | 1.00x | +| `tree` | 8.45-8.63 s | 8.47-8.55 s | ~1.00x | +| `deeplist` | 1.52 s | **1.53-1.54 s** | **0.987-0.993x** | + +Best-of-7 wall clock on the pinned quiet host at load < 2, two independent runs +per arm, `main` re-measured between them (it reproduced to the millisecond on +all three of its runs), and the whole set re-confirmed after rebasing onto +`main`'s newer head. **`deeplist` pays 0.7-1.3%**, reproducibly across three +runs: its nodes have pointer fields, so it takes the validating entry point, +whose per-slot loop now builds its two slices inside the loop's own branch +instead of finding them hoisted. + +The leaf profile moves the way the mechanism predicts: +`typed_shape_layout_entry` 22.7% + `layout_forget_object` 4.1% = **26.7% of +710 ms** becomes `init_typed_shape_layout` 12.6% + +`js_gc_declare_typed_shape_layout` 2.1% + `layout_forget_object` 5.6% = **20.4% +of 650 ms** — 190 ms down to 132 ms, against a 60 ms wall-clock improvement. + +**Binary size: +0 bytes, and structurally so.** The diff touches two files, both +in `perry-runtime`; no codegen crate is modified, so emitted IR is unchanged by +construction. Measured either way, all seven benchmark binaries are byte-for-byte +the same size as `main`'s (12,222,200 each), and the symbol-carrying build is +320 bytes *smaller*. + +### The codegen remedy the ticket proposed is unsound — do not revisit it + +Worth recording, because it looks free. The classes that take the `declare` path +must have an **empty pointer mask** (`class_layout_declarable_at_allocation`), so +their declared state is `GC_LAYOUT_POINTER_FREE` — byte-identical to what the +allocator already writes. Since #7566 a `new` inside a loop writes its `GcHeader` +as a single i64 constant store, so OR-ing `GC_OBJ_TYPED_LAYOUT_INTACT` into that +constant would set the bit for **+0 instructions and +0 bytes**. + +It would also be a use-after-free factory. Setting the bit without an installed +descriptor breaks the invariant "intact ⟹ some descriptor is reachable", and +`layout_note_slot` has a hole that only that invariant closes: on a contradicting +store to an intact-but-descriptor-less object it resolves a `None` verdict, falls +through to the ordinary pointer-mask bookkeeping, moves the object to +`SIDE_MASK` — and never clears the intact bit, because `layout_set_typed_unknown` +is reached only from the `Some(verdict)` arm. The object is then simultaneously +`SIDE_MASK` (the collector believes slot K holds a live pointer) and intact (the +codegen-inlined class-field guard in `expr/class_field_inline_guard.rs` believes +slot K is raw-f64). That guard consults no map by design, so it passes, and +`property_set.rs`'s raw-store fast path stores a double over the pointer **with +no write barrier and no layout note** — after which the next collection walks +slot K as a heap pointer. `layout_transfer` re-derives the bit correctly on +evacuation, but only for objects that are actually evacuated, so the window is +the object's lifetime. + +### Testing + +`cargo test -p perry-runtime` (1820) and `-p perry-codegen --lib` green; +`check_file_size.sh`, `addr_class_inventory.py`, `raw_handle_debt.py` (998, +unchanged) and `cargo fmt --all -- --check` clean; `gc_root_dominance_check.py` +in both gated modes with `--seeded-violations 40`, which this change cannot +affect since it emits no code. + +Two new tests, both sabotage-verified rather than merely run: + +- `the_pointer_mask_empty_bit_round_trips_per_entry` — the replayed bit must be + per-entry and must be the recorded one. Dropping it from `pack_dims` turns it + red, and so does making `hit` return a constant `Some(true)`, which also takes + down `memo_installed_objects_survive_a_copying_minor_with_their_children` (the + GC witness) and `a_memo_hit_produces_the_same_header_state_as_the_install`. +- `packed_dims_fields_do_not_overlap_the_empty_bit` — the word-count fields + narrowed from 20 bits to 19 to make room for bit 62; widening one back turns + it red. An overlap would make a wide-mask shape read back as `POINTER_FREE`, + and the collector would skip payload slots holding live pointers. + +The existing `a_contradicting_field_is_refused_even_with_the_memo_warm` +earned its keep: an earlier draft of this change probed the memo before the +per-slot validation, and that test caught it on the counter assertion its message +names. The probe now sits after validation, exactly where it was. diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index 0fc743406a..95788e0dc4 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/crates/perry-runtime/src/gc/layout.rs @@ -966,16 +966,43 @@ enum TypedShapeProof { FreshlyAllocated, } +/// Rebuild a mask slice from the raw `(pointer, word count)` pair the FFI +/// signature carries. +/// +/// #7578 keeps the construction path on the raw pair and materialises a slice +/// only where one is actually indexed. `slice::from_raw_parts` requires a +/// non-null aligned pointer, so every call used to open with two +/// null-to-`NonNull::dangling()` `csel` chains — twelve instructions to +/// normalise two arguments that the fast path below then never dereferences. +#[inline(always)] +unsafe fn mask_words<'a>(words: *const u64, word_count: u32) -> &'a [u64] { + if words.is_null() || word_count == 0 { + &[] + } else { + std::slice::from_raw_parts(words, word_count as usize) + } +} + +#[allow(clippy::too_many_arguments)] unsafe fn init_typed_shape_layout( user_ptr: usize, slot_count: usize, - raw_f64_words: &[u64], - pointer_words: &[u64], + raw_f64_words: *const u64, + raw_f64_word_count: u32, + pointer_words: *const u64, + pointer_word_count: u32, proof: TypedShapeProof, ) { - let Some(header) = layout_header_for_user(user_ptr) else { + // One `gc_type_layout_slot_kind`, not two. `layout_header_for_user` + // computes the kind and accepts three of them; the line that used to follow + // it recomputed the same kind — a second load through the 32-byte-strided + // type table — to narrow those three to one. Requiring `ObjectFields` + // directly is exactly equivalent, because `ObjectFields` is one of the + // three `layout_header_for_user` admits (#7578). + if user_ptr < GC_HEADER_SIZE + 0x1000 { return; - }; + } + let header = header_from_user_ptr(user_ptr as *const u8); if gc_type_layout_slot_kind((*header).obj_type) != GcLayoutSlotKind::ObjectFields { return; } @@ -986,18 +1013,9 @@ unsafe fn init_typed_shape_layout( return; } - // The two soundness checks, straight off the caller's mask words. Building - // a `LayoutSlotMask` here would be a 32-byte enum with a destructor — - // ×2, plus drop glue at every early return below, plus an allocation each - // for any shape wider than 64 slots — to answer two predicates. The - // descriptor that genuinely needs the type is built further down, only - // when a shape is actually installed. - if super::shape_install::words_intersect(raw_f64_words, pointer_words, slot_count) { - layout_set_typed_unknown(header, user_ptr); - return; - } - if slot_count != 0 && proof == TypedShapeProof::ValidateSlots { + let raw_f64_words = mask_words(raw_f64_words, raw_f64_word_count); + let pointer_words = mask_words(pointer_words, pointer_word_count); let fields = (obj_header as *const u8) .add(std::mem::size_of::()) as *const u64; @@ -1029,19 +1047,32 @@ unsafe fn init_typed_shape_layout( // no `TypedLayoutDescriptor` built, cloned and dropped, no `RefCell` // borrow, no hash of `keys`, no field-by-field descriptor comparison. // - // The memo carries no header state: `POINTER_FREE` vs `SIDE_MASK` is - // recomputed from the pointer mask exactly as the slow path computes it, - // so a stale entry can only cost work. See `gc::shape_install` for the - // full staleness argument. + // The memo carries no state about the OBJECT. The one bit it carries about + // the *masks* — whether the pointer mask is empty, which selects + // `POINTER_FREE` vs `SIDE_MASK` — is a pure function of bytes the entry has + // already matched by address and length, and those bytes are immutable + // program constants. See `gc::shape_install` for the full staleness + // argument. // - // `object_keys_array_ptr`'s two guards are already discharged above - // (`layout_header_for_user` rejected the low addresses, - // `GcLayoutSlotKind::ObjectFields` was checked), so read the field - // directly rather than re-walking the header. + // `object_keys_array_ptr`'s two guards are already discharged above (the + // low addresses were rejected, `GcLayoutSlotKind::ObjectFields` was + // checked), so read the field directly rather than re-walking the header. let keys = (*obj_header).keys_array as usize; - if keys != 0 && super::shape_install::hit(keys, slot_count, raw_f64_words, pointer_words) { + let memo = if keys == 0 { + None + } else { + super::shape_install::hit( + keys, + slot_count, + raw_f64_words, + raw_f64_word_count, + pointer_words, + pointer_word_count, + ) + }; + if let Some(pointer_mask_empty) = memo { header_set_typed_layout_intact(header); - if super::shape_install::words_are_empty(pointer_words) { + if pointer_mask_empty { set_layout_state(header, GC_LAYOUT_POINTER_FREE); } else { set_layout_state(header, GC_LAYOUT_SIDE_MASK); @@ -1050,10 +1081,27 @@ unsafe fn init_typed_shape_layout( return; } - let pointer_mask = LayoutSlotMask::from_words(pointer_words); + let raw_f64_slice = mask_words(raw_f64_words, raw_f64_word_count); + let pointer_slice = mask_words(pointer_words, pointer_word_count); + + // #7578: the mask-disjointness check moved down here, off the hit path. + // + // It is a pure function of the two mask globals, and a memo hit proves an + // install already ran it over the *same* globals and passed — a shape whose + // masks intersect is downgraded here and never reaches `record`, so no + // intersecting tuple can be in the table to hit. Running it above the probe + // charged every construction of every shape for a compile-time property of + // the class. It stays ahead of every install, which is the only place its + // answer is used. + if super::shape_install::words_intersect(raw_f64_slice, pointer_slice, slot_count) { + layout_set_typed_unknown(header, user_ptr); + return; + } + + let pointer_mask = LayoutSlotMask::from_words(pointer_slice); let descriptor = TypedLayoutDescriptor { slot_count, - raw_f64_mask: LayoutSlotMask::from_words(raw_f64_words), + raw_f64_mask: LayoutSlotMask::from_words(raw_f64_slice), pointer_mask: pointer_mask.clone(), }; // #6893: try the O(shapes) shared shape descriptor (keyed by the canonical @@ -1071,7 +1119,19 @@ unsafe fn init_typed_shape_layout( // canonical descriptor, so this object needs no per-object record at // all. `layout_forget_object` skips the hash entirely when the maps // are empty, which on a monomorphic workload they are. - super::shape_install::record(keys, slot_count, raw_f64_words, pointer_words); + // + // `pointer_mask.is_empty()` is what the hit path above will replay from + // the memo; `words_are_empty` is pinned equal to it by + // `shape_install::tests::mask_word_helpers_agree_with_layout_slot_mask`. + super::shape_install::record( + keys, + slot_count, + raw_f64_words, + raw_f64_word_count, + pointer_words, + pointer_word_count, + pointer_mask.is_empty(), + ); layout_forget_object(user_ptr); return; } @@ -1102,18 +1162,15 @@ fn typed_shape_layout_entry( return; } unsafe { - let raw_words: &[u64] = if raw_f64_mask_words.is_null() || raw_f64_mask_word_count == 0 { - &[] - } else { - std::slice::from_raw_parts(raw_f64_mask_words, raw_f64_mask_word_count as usize) - }; - let pointer_words: &[u64] = if pointer_mask_words.is_null() || pointer_mask_word_count == 0 - { - &[] - } else { - std::slice::from_raw_parts(pointer_mask_words, pointer_mask_word_count as usize) - }; - init_typed_shape_layout(user_ptr, slot_count, raw_words, pointer_words, proof); + init_typed_shape_layout( + user_ptr, + slot_count, + raw_f64_mask_words, + raw_f64_mask_word_count, + pointer_mask_words, + pointer_mask_word_count, + proof, + ); } } diff --git a/crates/perry-runtime/src/gc/shape_install.rs b/crates/perry-runtime/src/gc/shape_install.rs index a697898c3b..825a2295f6 100644 --- a/crates/perry-runtime/src/gc/shape_install.rs +++ b/crates/perry-runtime/src/gc/shape_install.rs @@ -34,19 +34,41 @@ //! //! # What the memo deliberately does NOT assert //! -//! **Anything about the object.** The caller still re-derives, from the mask -//! words themselves, every fact that its header declaration rests on: -//! `field_count == slot_count`, raw-f64/pointer mask disjointness, and the -//! per-slot validation that each raw-f64 slot holds a plain double and no -//! pointer-bearing slot sits outside the pointer mask. The -//! `POINTER_FREE`/`SIDE_MASK` choice is recomputed from the pointer mask, not -//! read back from the memo — the memo carries no header state at all. +//! **Anything about the object.** The caller still re-derives, per instance, +//! every fact its header declaration rests on that depends on the *object*: +//! `field_count == slot_count`, and — for the validating entry point — the +//! per-slot check that each raw-f64 slot holds a plain double and no +//! pointer-bearing slot sits outside the pointer mask. //! //! This split is the soundness bar. A wrong `POINTER_FREE` is a //! use-after-free factory: `heap_payload_slot_selection` skips the whole -//! payload without consulting any mask. Keeping that decision on data the -//! caller re-derives per object means a stale memo can only cost work, never -//! correctness. +//! payload without consulting any mask. Keeping every object-dependent +//! decision on data the caller re-derives per object means a stale memo can +//! only cost work, never correctness. +//! +//! # What the memo DOES replay, and why that is not the same thing (#7578) +//! +//! Two predicates over the mask words alone: their disjointness, and whether +//! the pointer mask is empty (the `POINTER_FREE`/`SIDE_MASK` choice). Both used +//! to be recomputed on every construction of every shape. +//! +//! Replaying them is sound for a reason that does not extend to anything about +//! the object. An entry matches on the mask globals' **addresses and lengths**, +//! and those globals are codegen-emitted `private unnamed_addr constant`s: they +//! live in the read-only image, are never written and are never freed. So a +//! matching address *is* a matching byte string, for the life of the process, +//! and a pure function of those bytes has one answer forever. Contrast a header +//! bit, which belongs to an object whose contents change under the mutator — +//! which is why every object-dependent check above stays per-instance. +//! +//! Disjointness needs no stored bit at all: a shape whose masks intersect is +//! downgraded before it can reach [`record`], so no intersecting tuple can be +//! in the table to hit. Emptiness is stored, in `dims` bit 62. +//! +//! The failure mode this leaves is a *miss*, not a wrong hit: two globals +//! holding equal words that LLVM declined to merge get separate entries, and +//! anything that fails the address, length or slot-count comparison falls +//! through to the install, which re-derives everything. //! //! # Self-healing //! @@ -175,9 +197,9 @@ const MEMO_SLOTS: usize = 32; /// /// `dims` packs `slot_count` (low 24 bits — `js_gc_init_typed_shape_layout` /// rejects anything ≥ 16_000_000 before reaching here), the raw-f64 mask word -/// count (bits 24..44) and the pointer mask word count (bits 44..64). -/// [`record`] refuses to store a tuple that does not fit, so a packed value is -/// never ambiguous. +/// count (bits 24..43), the pointer mask word count (bits 43..62) and, in bit +/// 62, whether the pointer mask is empty. [`record`] refuses to store a tuple +/// that does not fit, so a packed value is never ambiguous. #[derive(Clone, Copy)] struct Entry { keys: usize, @@ -197,13 +219,25 @@ const EMPTY: Entry = Entry { }; const SLOT_COUNT_BITS: u32 = 24; -const WORD_COUNT_BITS: u32 = 20; +const WORD_COUNT_BITS: u32 = 19; +/// Bit 62 of `dims`: this shape's pointer mask is empty, so its install +/// selected `GC_LAYOUT_POINTER_FREE` rather than `GC_LAYOUT_SIDE_MASK` (#7578). +/// +/// Payload, not key: [`hit`] probes with it clear and reads the stored one back +/// out of the matched entry. +const POINTER_MASK_EMPTY_BIT: u64 = 1 << 62; -/// Pack the dimension triple, or `None` when it does not fit (a pathological -/// shape: > 16M slots, or > 1M mask words). An unpackable tuple is simply -/// never memoised. +/// Pack the dimension triple plus the pointer-mask-empty bit, or `None` when it +/// does not fit (a pathological shape: > 16M slots, or > 512K mask words — +/// 33.5M slots, twice the ceiling the entry point already enforces). An +/// unpackable tuple is simply never memoised. #[inline] -fn pack_dims(slot_count: usize, raw_len: usize, pointer_len: usize) -> Option { +fn pack_dims( + slot_count: usize, + raw_len: usize, + pointer_len: usize, + pointer_mask_empty: bool, +) -> Option { if slot_count >= 1 << SLOT_COUNT_BITS || raw_len >= 1 << WORD_COUNT_BITS || pointer_len >= 1 << WORD_COUNT_BITS @@ -213,7 +247,12 @@ fn pack_dims(slot_count: usize, raw_len: usize, pointer_len: usize) -> Option &'static UnsafeCell<[Entry; MEMO_SLOTS]> { } } -/// True when `SHAPE_LAYOUTS[keys]` is already known to hold exactly the -/// descriptor that `(slot_count, raw_words, pointer_words)` describes. +/// `Some(pointer_mask_empty)` when `SHAPE_LAYOUTS[keys]` is already known to +/// hold exactly the descriptor that `(slot_count, raw_words, pointer_words)` +/// describes; `None` otherwise. /// -/// `false` is never an error — it just means the caller must take the ordinary +/// `None` is never an error — it just means the caller must take the ordinary /// `shape_install_shared` path, which is what establishes the entry. +/// +/// **Takes the raw `(pointer, word count)` pairs, not slices** (#7578). The +/// caller receives them that way across the FFI boundary, and normalising a +/// null pointer into `NonNull::dangling()` so that `slice::from_raw_parts` is +/// legal cost twelve instructions per construction to build two slices this +/// function only ever compares as integers. [`record`] takes the same pairs, so +/// the two agree on the empty-mask representation by construction. #[inline(always)] pub(super) fn hit( keys: usize, slot_count: usize, - raw_words: &[u64], - pointer_words: &[u64], -) -> bool { + raw_words: *const u64, + raw_word_count: u32, + pointer_words: *const u64, + pointer_word_count: u32, +) -> Option { debug_assert!(keys != 0, "the null keys_array is the empty-slot marker"); - let Some(dims) = pack_dims(slot_count, raw_words.len(), pointer_words.len()) else { - return false; - }; + let dims = pack_dims( + slot_count, + raw_word_count as usize, + pointer_word_count as usize, + false, + )?; // SAFETY: `table()` is this thread's own storage; the reference does not // escape and nothing re-enters between the read and its use. - let entry = - unsafe { (*table().get())[slot_index(keys, raw_words.as_ptr(), pointer_words.as_ptr())] }; + let entry = unsafe { (*table().get())[slot_index(keys, raw_words, pointer_words)] }; let matched = entry.keys == keys - && entry.dims == dims - && std::ptr::eq(entry.raw_words, raw_words.as_ptr()) - && std::ptr::eq(entry.pointer_words, pointer_words.as_ptr()); - #[cfg(test)] - { - if matched { - counters::note_hit(); - } + && entry.dims & !POINTER_MASK_EMPTY_BIT == dims + && std::ptr::eq(entry.raw_words, raw_words) + && std::ptr::eq(entry.pointer_words, pointer_words); + if !matched { + return None; } - matched + #[cfg(test)] + counters::note_hit(); + Some(entry.dims & POINTER_MASK_EMPTY_BIT != 0) } /// Record that `shape_install_shared` just confirmed (or established) /// `SHAPE_LAYOUTS[keys] == Some(D)` for the descriptor these mask words -/// describe. +/// describe, together with the `POINTER_FREE`/`SIDE_MASK` choice that +/// descriptor's pointer mask selects. #[inline] -pub(super) fn record(keys: usize, slot_count: usize, raw_words: &[u64], pointer_words: &[u64]) { +pub(super) fn record( + keys: usize, + slot_count: usize, + raw_words: *const u64, + raw_word_count: u32, + pointer_words: *const u64, + pointer_word_count: u32, + pointer_mask_empty: bool, +) { if keys == 0 { return; } - let Some(dims) = pack_dims(slot_count, raw_words.len(), pointer_words.len()) else { + let Some(dims) = pack_dims( + slot_count, + raw_word_count as usize, + pointer_word_count as usize, + pointer_mask_empty, + ) else { return; }; #[cfg(test)] counters::note_record(); // SAFETY: as in `hit` — this thread's own storage, no escaping reference. unsafe { - (*table().get())[slot_index(keys, raw_words.as_ptr(), pointer_words.as_ptr())] = Entry { + (*table().get())[slot_index(keys, raw_words, pointer_words)] = Entry { keys, - raw_words: raw_words.as_ptr(), - pointer_words: pointer_words.as_ptr(), + raw_words, + pointer_words, dims, }; } @@ -436,6 +500,30 @@ mod tests { } } + /// `record`/`hit` take raw `(pointer, word count)` pairs (#7578); these two + /// keep the tests reading like the slice API they replaced. + fn put(keys: usize, slot_count: usize, raw: &[u64], pointers: &[u64], empty: bool) { + record( + keys, + slot_count, + raw.as_ptr(), + raw.len() as u32, + pointers.as_ptr(), + pointers.len() as u32, + empty, + ); + } + fn get(keys: usize, slot_count: usize, raw: &[u64], pointers: &[u64]) -> Option { + hit( + keys, + slot_count, + raw.as_ptr(), + raw.len() as u32, + pointers.as_ptr(), + pointers.len() as u32, + ) + } + /// Distinct shapes must not alias into one entry, and a shape whose /// dimensions do not fit the packed key must simply never memoise rather /// than collide with one that does. @@ -447,21 +535,72 @@ mod tests { let keys = 0x4000usize; invalidate(); - record(keys, 2, &raw, &pointers); - assert!(hit(keys, 2, &raw, &pointers)); - assert!(!hit(keys + 16, 2, &raw, &pointers), "a different shape"); - assert!(!hit(keys, 3, &raw, &pointers), "a different slot count"); - assert!(!hit(keys, 2, &other_raw, &pointers), "a different raw mask"); - assert!(!hit(keys, 2, &raw, &raw), "a different pointer mask"); - assert!(!hit(keys, 2, &raw, &[]), "a different pointer word count"); + put(keys, 2, &raw, &pointers, false); + assert!(get(keys, 2, &raw, &pointers).is_some()); + assert!( + get(keys + 16, 2, &raw, &pointers).is_none(), + "a different shape" + ); + assert!( + get(keys, 3, &raw, &pointers).is_none(), + "a different slot count" + ); + assert!( + get(keys, 2, &other_raw, &pointers).is_none(), + "a different raw mask" + ); + assert!( + get(keys, 2, &raw, &raw).is_none(), + "a different pointer mask" + ); + assert!( + get(keys, 2, &raw, &[]).is_none(), + "a different pointer word count" + ); invalidate(); assert!( - !hit(keys, 2, &raw, &pointers), + get(keys, 2, &raw, &pointers).is_none(), "invalidate must drop entries" ); } + /// #7578: the memo now replays the `POINTER_FREE`/`SIDE_MASK` choice, so it + /// has to carry that bit **per entry** and hand back the one that was + /// recorded — not a default, and not the neighbouring shape's. + /// + /// The dangerous direction is a spurious `true`: `POINTER_FREE` makes + /// `heap_payload_slot_selection` skip the payload without consulting any + /// mask, so a shape with live pointer slots would have its children dropped. + #[test] + fn the_pointer_mask_empty_bit_round_trips_per_entry() { + let raw: [u64; 1] = [0b01]; + let pointers: [u64; 1] = [0b10]; + let empty_pointers: [u64; 1] = [0]; + let keys_masked = 0xC000usize; + let keys_free = 0xD000usize; + + invalidate(); + put(keys_masked, 2, &raw, &pointers, false); + put(keys_free, 2, &raw, &empty_pointers, true); + + assert_eq!( + get(keys_masked, 2, &raw, &pointers), + Some(false), + "a shape with a live pointer mask must replay SIDE_MASK" + ); + assert_eq!( + get(keys_free, 2, &raw, &empty_pointers), + Some(true), + "a shape with an empty pointer mask must replay POINTER_FREE" + ); + + // The bit is payload, not key: it must not make a matching tuple miss. + invalidate(); + put(keys_free, 2, &raw, &empty_pointers, true); + assert_eq!(get(keys_free, 2, &raw, &empty_pointers), Some(true)); + } + /// A `slot_count` that overflows the packed key is refused by both halves, /// so it can neither be stored nor produce a spurious hit against a /// smaller shape that packs to the same bits. @@ -472,9 +611,40 @@ mod tests { let huge = 1usize << SLOT_COUNT_BITS; invalidate(); - record(keys, huge, &raw, &[]); - assert!(!hit(keys, huge, &raw, &[])); + put(keys, huge, &raw, &[], true); + assert!(get(keys, huge, &raw, &[]).is_none()); // …and it did not land in the slot a packable shape would use. - assert!(!hit(keys, 0, &raw, &[])); + assert!(get(keys, 0, &raw, &[]).is_none()); + } + + /// The word-count fields narrowed from 20 bits to 19 to make room for + /// [`POINTER_MASK_EMPTY_BIT`] (#7578). Pin the packing so a future widening + /// of any field cannot silently overlap that bit: an overlap would make a + /// wide-mask shape read back as `POINTER_FREE`, and the collector would + /// then skip payload slots that hold live pointers. + #[test] + fn packed_dims_fields_do_not_overlap_the_empty_bit() { + let max_slots = (1usize << SLOT_COUNT_BITS) - 1; + let max_words = (1usize << WORD_COUNT_BITS) - 1; + let packed = pack_dims(max_slots, max_words, max_words, false) + .expect("the maximum packable tuple must pack"); + assert_eq!( + packed & POINTER_MASK_EMPTY_BIT, + 0, + "a maximal dimension triple must leave the empty bit clear" + ); + assert_eq!( + pack_dims(max_slots, max_words, max_words, true), + Some(packed | POINTER_MASK_EMPTY_BIT), + "setting the empty bit must be the only difference" + ); + assert!( + pack_dims(max_slots, 1 << WORD_COUNT_BITS, 0, false).is_none(), + "an unpackable raw word count must be refused, not truncated" + ); + assert!( + pack_dims(max_slots, 0, 1 << WORD_COUNT_BITS, false).is_none(), + "an unpackable pointer word count must be refused, not truncated" + ); } }