feat(runtime): gate cold-path diagnostic JSON serializers behind diagnostics (binary size)#5159
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughIntroduces a Changesdiagnostics Cargo feature: detection, gating, and build wiring
Android NDK path formatting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…gnostics`
The GC cycle-telemetry (`PERRY_GC_DIAG`), typed-feedback trace dump
(`PERRY_TYPED_FEEDBACK`), v8 heap-snapshot builder
(`v8.getHeapSnapshot`/`writeHeapSnapshot`), and `process.report` JSON
serializers are never on a hot path. Gate them — and the `serde_json` graph
reachable only through them, which then dead-strips — behind a new
`diagnostics` cargo feature (in `default`). A `console.log` hello-world drops
~95 KB.
The compiler enables `perry-runtime/diagnostics` for the auto-optimize build
only when a program uses a heap-snapshot API or `process.report` (detected in
collect_modules; forwarded + cache-keyed in optimized_libs). The env-driven
dev diagnostics (GC-diag / typed-feedback JSON) ride the same feature and
degrade gracefully when off (terse non-JSON line / `"{}"` stub), so they're
absent from size-optimized binaries — use a full build for those.
`js_typed_feedback_maybe_dump_trace` stays an always-compiled extern (codegen
emits an unconditional call from `main`); only its JSON-building body is gated.
No speed change — pure dead-code elimination on cold paths.
Contributor
Author
|
Note: this PR also reformats |
proggeramlug
force-pushed
the
feat/binary-size-diagnostics
branch
from
a87b80f to
4dadda8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/perry-runtime/src/typed_feedback/trace.rs`:
- Around line 390-391: The `#[used]` attribute on the static variable K23 that
anchors the `js_typed_feedback_maybe_dump_trace` function is conditionally
compiled only when the "diagnostics" feature is enabled, but the generated code
unconditionally calls this function from main. Remove the `#[cfg(feature =
"diagnostics")]` guard so the `#[used]` attribute is always applied regardless
of feature flags, ensuring the symbol is retained in all builds including
non-diagnostics builds where LTO might otherwise dead-strip the symbol and cause
link failures.
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Around line 1939-1943: The current substring match for "property: \"report\""
in the hir_debug check is too broad and will match any report property, not just
process.report. Replace this raw substring check with logic that specifically
detects when the report property is accessed on the global process object
(verify the receiver is the global process identifier). This ensures
ctx.uses_diagnostics is only set to true for actual process.report calls,
preventing incorrect over-enabling of perry-runtime/diagnostics for unrelated
report properties.
🪄 Autofix (Beta)
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: 7741173a-6d66-41fd-ae7a-2717a72bdd70
📒 Files selected for processing (18)
crates/perry-runtime/Cargo.tomlcrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/walk.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/malloc.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/types.rscrates/perry-runtime/src/node_v8.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/typed_feedback.rscrates/perry-runtime/src/typed_feedback/trace.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/types.rs
Comment on lines
+390
to
391
| #[cfg(feature = "diagnostics")] | ||
| #[used] static K23: extern "C" fn() = js_typed_feedback_maybe_dump_trace; |
There was a problem hiding this comment.
Keep js_typed_feedback_maybe_dump_trace anchored in non-diagnostics builds.
Line 390 conditionally removes the #[used] retention anchor, but Line 311 says codegen still emits an unconditional call from main. In the auto-optimize/LTO path this can dead-strip the symbol and fail final link with an undefined external.
Suggested fix
- #[cfg(feature = "diagnostics")]
#[used] static K23: extern "C" fn() = js_typed_feedback_maybe_dump_trace;📝 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
| #[cfg(feature = "diagnostics")] | |
| #[used] static K23: extern "C" fn() = js_typed_feedback_maybe_dump_trace; | |
| #[used] static K23: extern "C" fn() = js_typed_feedback_maybe_dump_trace; |
🤖 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/typed_feedback/trace.rs` around lines 390 - 391, The
`#[used]` attribute on the static variable K23 that anchors the
`js_typed_feedback_maybe_dump_trace` function is conditionally compiled only
when the "diagnostics" feature is enabled, but the generated code
unconditionally calls this function from main. Remove the `#[cfg(feature =
"diagnostics")]` guard so the `#[used]` attribute is always applied regardless
of feature flags, ensuring the symbol is retained in all builds including
non-diagnostics builds where LTO might otherwise dead-strip the symbol and cause
link failures.
Comment on lines
+1939
to
+1943
| if hir_debug.contains("method: \"getHeapSnapshot\"") | ||
| || hir_debug.contains("method: \"writeHeapSnapshot\"") | ||
| || hir_debug.contains("property: \"report\"") | ||
| { | ||
| ctx.uses_diagnostics = true; |
There was a problem hiding this comment.
Narrow report detection to process.report only.
The current property: "report" match will flip uses_diagnostics for unrelated .report properties. That over-enables perry-runtime/diagnostics and defeats size pruning on non-diagnostics apps.
Suggested fix direction
- if hir_debug.contains("method: \"getHeapSnapshot\"")
- || hir_debug.contains("method: \"writeHeapSnapshot\"")
- || hir_debug.contains("property: \"report\"")
- {
- ctx.uses_diagnostics = true;
- }
+ if hir_debug.contains("method: \"getHeapSnapshot\"")
+ || hir_debug.contains("method: \"writeHeapSnapshot\"")
+ || module_uses_process_report(&hir_module)
+ {
+ ctx.uses_diagnostics = true;
+ }Use a structured HIR walk for process.report (receiver must be global process), instead of a raw substring.
🤖 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/src/commands/compile/collect_modules.rs` around lines 1939 -
1943, The current substring match for "property: \"report\"" in the hir_debug
check is too broad and will match any report property, not just process.report.
Replace this raw substring check with logic that specifically detects when the
report property is accessed on the global process object (verify the receiver is
the global process identifier). This ensures ctx.uses_diagnostics is only set to
true for actual process.report calls, preventing incorrect over-enabling of
perry-runtime/diagnostics for unrelated report properties.
This was referenced Jun 15, 2026
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.
What
Gate the four cold-path diagnostic JSON serializers behind a new
diagnosticscargo feature (indefault), so binaries that don't use them — and theserde_jsongraph reachable only through them — dead-strip:PERRY_GC_DIAG)PERRY_TYPED_FEEDBACK)v8.getHeapSnapshot/writeHeapSnapshot)process.reportResult
A
console.loghello-world drops ~95 KB (5.5 → 5.4 MB). No speed change — pure dead-code elimination on cold paths (opt-leveluntouched).Verified: hello-world links + runs at 5.4 MB with
diagnosticsoff; a program callingv8.getHeapSnapshot()+process.report.getReport()auto-enables the feature and returns real objects.How
perry-runtime/diagnosticsfor the auto-optimize build only when a program uses a heap-snapshot API orprocess.report(detected incollect_modulesviamethod: "getHeapSnapshot"/"writeHeapSnapshot"/property: "report"; forwarded + cache-keyed inoptimized_libs)."{}"stub) — absent from size-optimized binaries; use a full build for those.js_typed_feedback_maybe_dump_tracestays an always-compiled extern (codegen emits an unconditional call frommain); only its JSON-building body is gated.The feature is in
default, so plaincargo build/cargo test --workspace/ the shipped prebuilt keep all diagnostics.Summary by CodeRabbit
Release Notes
New Features
process.reportAPIs are now available by default.Chores