fix(hir): keep large JSON defines as serialized data - #10161
proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughLarge JSON-compatible object and array literals now use serialized JSON parsing at evaluation time when they exceed shape-specific thresholds. Unsupported JavaScript semantics retain ordinary lowering. Tests, documentation, and benchmarks cover the new behavior. ChangesLarge JSON literal lowering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Compiler
participant json_literal
participant GeneratedCode
participant JSONParser
Compiler->>json_literal: inspect object or array literal
json_literal->>GeneratedCode: emit serialized string and JsonParse expression
GeneratedCode->>JSONParser: parse serialized value at evaluation site
JSONParser-->>GeneratedCode: return fresh object or array
Merge Risk: ⚪ Minimal · up to No unresolved issue remains that should block merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 5 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 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 |
|
Audit of head Validation (train branch = this head + version bump, cherry-picked onto
The scope matters here. The switch is in
The record slowdown is not new code. On main, the same loop over Options as I see them:
Given the standing "keep best compute" rule, I'm not landing a 2.5× hot-path regression unilaterally. Tell me which option to take, or push a revision, and I'll land it. |
|
Implemented Option 2 in 699ae8f.
Cutoff calibration used the audit's exact Five interleaved Linux A/B runs with the final compiler and matching archives:
Re-audit caveat: the requested combined-file numeric speedup is not retained. The old broad rule measured 71 ms / 938 ms in that file. The revision fixes the record-read regression, but ordinary record initialization pushes the same unit over LLVM's existing 100k-instruction O0 machine-code limit (~622k instructions), slowing the numeric loop too. HIR confirms that its numeric array still uses The preserved 4,623,800-byte / 213-provider OpenCode define compiles and links in 1.52 s, printing 213. The installed snapshot has refreshed to 4,637,074 bytes, still 213 providers; it takes 1.54 s and also prints 213. The actual All via |
|
Landed via merge train #10172 (rebase-merged; main |
Summary
Fix multi-MB JSON defines that never finish LLVM code generation, while preserving ordinary lowering and static property layouts for mid-size records (Option 2 from the review).
The original 4,623,800-byte OpenCode snapshot expanded into 395 synthetic classes and 772,701,328 bytes of saved LLVM IR across 30 units. HIR-to-LLVM emission took roughly 11 seconds; the last LLVM unit did not finish in 600 seconds. Its entry function reached 8,445,662 instructions, with peak RSS of 13,667,064 KiB. The stall is in LLVM after literal expansion, not an AST-to-HIR lowering loop.
Changes
Expr::JsonParse(Expr::String(...))at the original evaluation site, matching the JSON-import intrinsic from perf(compile): reduce generated bundle bloat #8418. Each evaluation creates a fresh value; untaken branches do not parse. Existing define substitution,typeoffolding, and both define-dependent cache keys are unchanged.The record-array cutoff was measured using the exact regressing shape (
id,name,tags,w), with a freshly built ordinary-lowering control equivalent to base8a058e2053:At 6,400 records, emission completed in 4.5 seconds with approximately 85.1 MiB estimated IR and a 1,446,386-instruction function before optimization; four of five LLVM units completed within a second. The remaining unit timed out. The selected node threshold switches this shape at 3,511 records, 27% below the eight-minute case and 45% below the timeout. It is 24 times the old node threshold; the text threshold is 16 times larger.
Related issue
Fixes #10151
Test plan
All builds and tests ran through
./remote.shon the Linux host with LLVM 22.1.8. None ran on the Mac. The compiler and matching runtime/stdlib archives were copied together out of the shared Cargo target. The ordinary control was built with the compact hook removed; the final compiler uses the code in this revision. Both link the same runtime/stdlib sources and archives.Final build/check commands (inside
./remote.sh):Reproduction commands, with each snapshot stored as the JS-expression value of
BIGin its directory'sperry.json:The preserved 4,623,800-byte snapshot compiles and links in 1.52 s, printing 213. The installed snapshot has since refreshed to 4,637,074 bytes (still 213 providers); it compiles and links in 1.54 s, also printing 213. Both use fresh caches. Peak RSS is about 410 MiB.
Real OpenCode verification keeps the installed
perry.jsonunchanged and uses a temporary entry inpackages/opencodeimporting../core/src/models-dev:The final compiler regenerated
packages/core/src/models-dev.tsin 10.6 s (its matching object was held out of the cache to force codegen). The surrounding compile timed out at 600 s later in unrelated Effect dependency codegen. Resuming with the same final compiler and object cache completed all 621 native modules, exit 0, in 60.42 s:/usr/bin/time -v timeout 900 env -u PERRY_NO_CACHE -u PERRY_DISABLE_BUILD_CACHE PERRY_MODULE_JOBS=8 PERRY_DISABLE_WELL_KNOWN=1 PERRY_CODEGEN_PROGRESS=1 "$PERRY_BIN" compile perry-10151-models.ts --platform bun --no-link --no-auto-optimize --output /tmp/def/two-tier/oc-final/main.o --cache-dir /tmp/def/oc-models-cacheAn additional ordinary-lowering 3,200-record probe exceeded its 180-second diagnostic timeout on the busy host. The selected threshold is a data-size heuristic below the measured 4,800/6,400-record cliff, not a universal compilation deadline for all smaller programs.
The no-link pipeline still resolves linker support archives;
PERRY_DISABLE_WELL_KNOWN=1avoids building those unrelated archives. The temporary entry is removed afterward. The full 7,800-module application was not built.Runtime A/B uses
benchmarks/large_json_literals/generate.py, the exact audit fixture, separate caches, and five interleaved runs. Commands and cutoff reproduction are documented in that directory's README.Performance limitation: the old broad rule measured 71 ms / 938 ms in the combined file. This revision restores record-read performance, but does not retain that combined file's numeric speedup. HIR confirms the numeric array still takes
JsonParse; ordinary record initialization makes the same unit exceed LLVM's existing 100k-instruction O0 machine-code limit (about 622k instructions). The independent number probe shows no intrinsic runtime improvement on this host. Changing initialization/unit splitting would be additional codegen work; this revision keeps the requested two-tier scope. This limitation is reported explicitly for re-audit.All numeric and record checksums match. The original semantic probe (mutations, key order, fresh arrays, Float64Array and Map) produced identical stdout across the ordinary control, old broad rule and revised rule. The combined file takes 58.90 s to compile under the final rule versus 60.07 s for ordinary lowering; mid-size record codegen is intentionally retained.
Checklist
Summary by CodeRabbit
Performance
Bug Fixes
Documentation