[EXPERIMENT] Decouple props from JS source: pass as separate JSON field#3294
[EXPERIMENT] Decouple props from JS source: pass as separate JSON field#3294AbanoubGhadban wants to merge 2 commits into
Conversation
Props (up to 1.1 MB for mega benchmark) are no longer embedded as a JS literal in the IIFE source string. Instead they travel as a separate `props` form field and are injected into the VM context via JSON.parse, which uses V8's optimized JSON parser instead of its JavaScript parser. Ruby side: JsCodeBuilder reads props from globalThis.__rorpProps (with fallback to IIFE param for RSC re-invocations). Thread-local carries the props string to Request.form_with_code. NodeStrategy.ensure cleans up the thread-local on all code paths. Node side: worker.ts extracts body.props, threads it through handleRenderRequest to vm.ts runInVM, which does context.__rorpProps = JSON.parse(propsJson) inside the try block and cleans up in finally, matching the existing pattern for renderingRequest/sharedExecutionContext. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The initial commit modified JsCodeBuilder, which is part of the new strategy pattern (issue #2905) not yet wired into the main rendering path. The actual production path uses ServerRenderingJsCode#render. - Set thread-local in ServerRenderingJsCode#render (the real entry point) - Change props embedding from inline `#{props_string}` to `globalThis.__rorpProps` - Remove debug instrumentation from request.rb Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb03b6c26
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) | ||
| form["props"] = props | ||
| Thread.current[:ror_decoupled_props] = nil |
There was a problem hiding this comment.
When the renderer returns STATUS_SEND_BUNDLE, NodeRenderingPool.eval_js retries the same js_code with send_bundle: true instead of regenerating it, but this line clears the only saved props after building the first form. On a cold renderer or after a bundle hash change, the retry uploads the bundle without a props field, so the JS now runs with globalThis.__rorpProps undefined and renders with missing props. Keep the props available for every retry attempt, or pass them explicitly rather than consuming the thread-local here.
Useful? React with 👍 / 👎.
| #{ssr_pre_hook_js} | ||
| #{redux_stores} | ||
| var usedProps = typeof props === 'undefined' ? #{props_string} : props; | ||
| var usedProps = typeof props === 'undefined' ? globalThis.__rorpProps : props; |
There was a problem hiding this comment.
This replacement makes every Pro render request depend on the Node VM injecting __rorpProps, but several supported paths never do that injection: Pro configured with server_renderer = "ExecJS" or the ExecJS fallback executes this JS directly, and the incremental-render NDJSON path builds its initial chunk without a props field. In those contexts, components receive undefined props instead of the serialized props_string; either keep inline props for non-Node-form transports or teach those transports to provide __rorpProps too.
Useful? React with 👍 / 👎.
Code Review: [EXPERIMENT] Decouple props from JS sourceThe performance motivation is solid — the benchmark results (50% reduction in VM execution time for large payloads) are compelling. The implementation is clearly labeled as experimental, and the caveats in the PR description are honest. Here are the issues to resolve before this could land in production. 🔴 Bugs1. Incremental rendering path is broken
2. When 3. If 🟡 Design concerns4. Silent failure when thread-local is not set If 5. Double-clearing of thread-local
🔵 Minor6. Type cast in const { renderingRequest, props: propsJson } = body as Record<string, unknown>;
7. Removed explanatory comments Two useful comments were deleted in
The second one in particular helps readers understand the backward-compatibility invariant that makes this whole approach safe. Consider keeping it. SummaryThe optimization is real and worth pursuing. Before this exits experiment status:
As noted in the PR description, replacing thread-locals with proper method signatures is the right long-term path; that work will also naturally fix issues 1 and 4. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| try { | ||
| if (propsJson) { | ||
| context.__rorpProps = JSON.parse(propsJson); |
There was a problem hiding this comment.
Missing error handling — if propsJson contains invalid JSON (encoding issue, truncated body, caller bug), this throws an opaque error with no indication that props parsing was the cause. Suggest wrapping with a descriptive catch:
| context.__rorpProps = JSON.parse(propsJson); | |
| try { | |
| context.__rorpProps = JSON.parse(propsJson); | |
| } catch (e) { | |
| throw new Error(`Failed to parse decoupled propsJson: ${e instanceof Error ? e.message : String(e)}`); | |
| } |
| @@ -397,6 +403,9 @@ export async function buildExecutionContext( | |||
| }; | |||
There was a problem hiding this comment.
runOnOtherBundle calls runInVM without forwarding propsJson. This is safe today because generateRSCPayload always embeds props directly in the IIFE call (so props !== undefined in the RSC bundle), but it's a silent invariant. Any future caller that relies on globalThis.__rorpProps in the RSC bundle will silently get undefined props. At minimum, add a comment here documenting the assumption; ideally forward propsJson explicitly:
| }; | |
| context.runOnOtherBundle = (bundleTimestamp: string | number, newRenderingRequest: string) => { | |
| const otherBundleFilePath = getRequestBundleFilePath(bundleTimestamp); | |
| // propsJson is intentionally not forwarded: generateRSCPayload embeds props | |
| // directly in the IIFE call, so the RSC bundle never needs __rorpProps. | |
| return runInVM(newRenderingRequest, otherBundleFilePath, vmCluster); | |
| }; |
| def form_with_code(js_code, send_bundle) | ||
| form = common_form_data | ||
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) |
There was a problem hiding this comment.
If Thread.current[:ror_decoupled_props] is nil here (not set by render(), cleared early, or called from an unexpected code path), the props field is silently omitted from the form. The node renderer then sees no propsJson, skips setting context.__rorpProps, and the component renders with undefined props — no exception, no log line.
Consider adding an assertion or a warning log when the rendering request contains globalThis.__rorpProps but the thread-local is absent, so failures are observable:
# If the js_code references __rorpProps but the thread-local wasn't set,
# that's a programmer error — raise early rather than silently pass undefined props.
size-limit report 📦
|
Greptile SummaryThis experimental PR decouples React component props from the JavaScript IIFE source string sent to the Node renderer, sending them as a separate HTTP form field (
Confidence Score: 3/5The experiment is intentionally rough-edged and not wired into the main production path yet, but the thread-local plumbing has real gaps that would need closing before a production merge. The
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| if (propsJson) { | ||
| context.__rorpProps = JSON.parse(propsJson); | ||
| } |
There was a problem hiding this comment.
if (propsJson) treats an empty string "" as "no props" and skips JSON.parse. While valid serialized props are never an empty string, an explicit != null guard documents the intent clearly and avoids potential silent failures if callers ever send an empty field.
| try { | |
| if (propsJson) { | |
| context.__rorpProps = JSON.parse(propsJson); | |
| } | |
| try { | |
| if (propsJson != null) { | |
| context.__rorpProps = JSON.parse(propsJson); | |
| } |
| def render(props_string, rails_context, redux_stores, react_component_name, render_options) | ||
| Thread.current[:ror_decoupled_props] = props_string |
There was a problem hiding this comment.
ensure to clean up thread-local in the main rendering path
Thread.current[:ror_decoupled_props] is set here at the very start of render. The only cleanup on the main (non-NodeStrategy) path lives inside the if (props = Thread.current[:ror_decoupled_props]) block inside form_with_code. If an exception propagates after this assignment but before form_with_code is called (e.g., a connection-level error raised inside the rendering pool before the HTTP form is built), the thread-local persists for the lifetime of the thread. On the next render on the same thread the value is overwritten, so the cross-contamination window is narrow — but if form_with_code is invoked again (e.g., a retry path in the rendering pool) without a preceding render, it will attach the stale props to the retry request silently.
| def form_with_code(js_code, send_bundle) | ||
| form = common_form_data | ||
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) | ||
| form["props"] = props | ||
| Thread.current[:ror_decoupled_props] = nil | ||
| end | ||
| populate_form_with_bundle_and_assets(form, check_bundle: false) if send_bundle | ||
| form | ||
| end |
There was a problem hiding this comment.
render_code_as_stream path
render_code_as_stream calls form_with_code inside a StreamRequest.create block. If that block is executed in the same OS thread (e.g., as an Async fiber), the read in line 283 is safe. However, if StreamRequest.create ever defers or migrates the block to a different thread (e.g., a thread-pool-backed async executor), Thread.current[:ror_decoupled_props] would be nil in the worker thread even though it is set in the calling thread, causing props to be silently dropped. The current Async/fiber implementation appears safe, but documenting the assumption (or using a Fiber.current local instead of a thread-local) would make this more robust.
|
Closing in favor of a plan-only PR. The experiment code changes will be reimplemented based on the plan document. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Experiment to measure the performance impact of decoupling component props from the JavaScript source string sent to the Node renderer.
Before: Props (~1.1 MB for mega benchmark) are embedded as a JS object literal inside the IIFE source string. V8 must re-parse the entire string on every request because
domNodeId(a random UUID) makes each source string unique — V8's compile cache cannot help.After: Props are sent as a separate HTTP form field. The IIFE source shrinks from ~1.1 MB to ~1.3 KB (stable template). On the Node side,
JSON.parse()parses the props string using V8's optimized JSON parser instead of the JS parser.Changes
Ruby side
ServerRenderingJsCode#render— SetThread.current[:ror_decoupled_props]with the props string; replace inline#{props_string}withglobalThis.__rorpPropsRequest#form_with_code— Pick up the thread-local and add apropsform field to the HTTP requestJsCodeBuilder— Same changes for the new strategy pattern path (not yet wired in)NodeStrategy#execute— Ensure cleanup of thread-localNode side
worker.ts— Extractbody.propsfrom the HTTP requesthandleRenderRequest.ts— ThreadpropsJsonparameter through to VM executionvm.ts— Injectcontext.__rorpProps = JSON.parse(propsJson)beforevm.runInContext(), clean up infinallyRSC compatibility
The
generateRSCPayloadfunction modifies the IIFE's trailing()to pass(componentName, propsString). With the change:typeof props === 'undefined' ? globalThis.__rorpProps : props— when called viagenerateRSCPayload,props !== undefinedso the IIFE parameter is used, not the global. This preserves backward compatibility.Benchmark Results
Microbenchmark: V8 JS parser vs JSON.parse (corrected)
Initial microbenchmark was flawed — it passed the same source string repeatedly, which let V8 reuse compiled code (cache hit). In production,
domNodeIdis a random UUID on every request, making every source string unique. Corrected benchmark uses varying source per iteration:Script:
/tmp/bench-json-vs-js-v2.mjs(500 warmup, 500 measured runs per size)End-to-end VM-level measurement (1.1 MB props, n=50)
Instrumented
vm.runInContext()in the Node renderer to measure actual execution time:Key finding
Every request produces a unique JS source string (due to random
domNodeId), so V8 cannot cache compiled code across requests. Decoupling props means:JSON.parse()which is ~5x faster than V8's JS parser for large payloadsRecommendations
This optimization is worth pursuing for production. ~18ms savings per request scales linearly with props size. For apps with large SSR payloads, this is meaningful.
The thread-local pattern should be replaced with proper method signatures before merging to production. It was used here to minimize changes across 4+ method layers for the experiment.
Consider also decoupling
railsContext— it's small (~1-2 KB) so savings would be minimal, but it would further stabilize the IIFE template for V8 caching.Consider setting
random_dom_id: false(or using a fixedid:) for SSR-heavy pages. This would allow V8 to cache the compiled IIFE across requests even without this optimization — but only when props don't change between requests.The
vm.ScriptAPI could provide additional gains — pre-compiling the stable IIFE template once and reusing it across requests, rather than passing source strings tovm.runInContext()each time.Test plan
🤖 Generated with Claude Code