Skip to content

docs: reframe performance benchmarks around ShakaPerf twin-server proof loop - #4766

Merged
justin808 merged 3 commits into
mainfrom
claude/rails-perf-benchmarks-docs-5450bd
Jul 21, 2026
Merged

docs: reframe performance benchmarks around ShakaPerf twin-server proof loop#4766
justin808 merged 3 commits into
mainfrom
claude/rails-perf-benchmarks-docs-5450bd

Conversation

@justin808

@justin808 justin808 commented Jul 21, 2026

Copy link
Copy Markdown
Member

What

Reframes docs/oss/core-concepts/performance-benchmarks.md around a single thesis: make a page measurably faster and prove the win — increasingly with an AI agent doing the conversion while a paired A/B benchmark keeps it honest — instead of the previous lab-notebook of collected numbers.

Also gives the page the complete look-over requested for the React 19.2 / RSC / React on Rails v17 era, folding in the factual fixes an audit surfaced.

Why

The page's longest section (~150 lines, ~35% of it) documented an April 2026 Gumroad-style benchmark it explicitly told readers not to trust (unknown RAILS_ENV, n=8, a responseEnd p95 counter-signal). All five of its tracking issues (#3128, #3144, #3253, #3259, #3263) are now closed — #3253 and #3263 as NOT_PLANNED — and the paired-sampling methodology it apologized for is exactly what ShakaPerf now provides. The page was spending its bulk caveating data instead of helping a reader improve and verify a page.

Changes

New spine

  • The improve-and-prove loop — prompt an agent → it converts the page → a paired A/B proves the win. Explains why step 3 has to be noise-resistant: an agent iterates on the same machine it benchmarks.
  • Proving a change with ShakaPerf — twin dockerized servers (control branch vs experiment branch, both production mode), paired simultaneous sampling (Wilcoxon signed-rank + Hodges-Lehmann) that cancels laptop/CI noise so no quiet machine is needed, what it measures (Web Vitals, JS bytes, hydration-start/end marks, visual regression, a11y), how to run it, reports, and source-available licensing. ShakaPerf is public on npm and GitHub.
  • Performance levers — the tuning knobs ranked by payoff, each linking its deep doc (Node renderer, RSC, streaming, caching, hydration scheduling, React Compiler, resource hints).
  • Keeps the warm-SSR-cache benchmarking guidance under a stable #benchmarking-rsc-against-warm-ssr-caches anchor (external docs link to it).

Staleness fixes

  • Corrects the selective-hydration note: Pro apps on Shakapacker ≥ 8.2.0 already default to :async (the page was telling every reader — all of them Pro, since streaming is Pro-only — to set a no-op, and the value is unsupported for non-Pro). :defer is the non-Pro default; :sync on Shakapacker < 8.2.0. Matches configuration.rb:181.
  • Resolves the ExecJS table contradiction ("new process per eval" vs "Comparable") and stops asserting an unsourced 3-10x figure; anchors on the measured Popmenu 73% reduction and calls the multiple workload-dependent.
  • Replaces the Gumroad section with a short, honest pointer.
  • Fixes the JSON-gem Ruby framing: v17 requires Ruby ≥ 3.3; Ruby 3.4+ ships json 2.9+ so the upgrade is unnecessary there.
  • Expands cross-links (caching, hydration scheduling, profiling, web-vitals, bundle caching, React Compiler).

Intentionally out of scope (not covered by ShakaPerf's page-side A/B): the benchmarks/ self-hosted Bencher trend (#4073) is maintainer-facing release-quality authority, so it stays in the repo docs, not this user-facing page.

Verification

  • All 16 relative links resolve; external #benchmarking-rsc-against-warm-ssr-caches dependency preserved; no external doc references the removed anchors.
  • prettier --check clean; script/check-docs-sidebar clean.
  • llms-full.txt regenerated and --check/--validate clean (llms-full-pro.txt untouched — OSS doc).
  • Net 423 → 406 lines, denser and reorganized.

Summary by CodeRabbit

  • Documentation
    • Reorganized and refreshed the React on Rails performance benchmarking guide around an “improve-and-prove” workflow.
    • Added a detailed ShakaPerf-based paired A/B methodology, including required cache-state controls (cold vs warm), comparison guidance, and where reports land.
    • Clarified SSR engine strategy comparisons (ExecJS vs Node Renderer) and strengthened React Server Components guidance (no hydration for server components), including Streaming SSR + selective hydration behavior.
    • Expanded guidance on fairness (apples-to-oranges), key delivery priorities (CSS/fonts/hero), JSON serialization performance recommendations, and updated examples/resources.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 701ad353-ea37-47d5-8721-4140e30110af

📥 Commits

Reviewing files that changed from the base of the PR and between 988b155 and f6d3261.

📒 Files selected for processing (2)
  • docs/oss/core-concepts/performance-benchmarks.md
  • llms-full.txt
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/oss/core-concepts/performance-benchmarks.md
  • llms-full.txt

Walkthrough

Reworked performance benchmarking documentation around an improve-and-prove workflow, paired ShakaPerf A/B testing, React Server Components guidance, cache-state controls, metric interpretation, real-world examples, measurement tools, and JSON serialization performance.

Changes

Performance benchmarking documentation

Layer / File(s) Summary
Improve-and-prove benchmark methodology
docs/oss/core-concepts/performance-benchmarks.md, llms-full.txt
Introduces the improve-and-prove loop and documents ShakaPerf paired A/B testing with twin servers, collected metrics, commands, reports, configuration, and an in-repository example.
SSR and RSC performance levers
docs/oss/core-concepts/performance-benchmarks.md, llms-full.txt
Updates ExecJS versus Node Renderer guidance and explains RSC JavaScript reduction, hydration boundaries, streaming, caching, code splitting, and related levers.
Result interpretation and experiment controls
docs/oss/core-concepts/performance-benchmarks.md, llms-full.txt
Adds metric decomposition, warm-cache baseline guidance, parity requirements, cold/warm SSR and RSC variants, payload pairing, and comparison controls.
Results, measurement, and reference updates
docs/oss/core-concepts/performance-benchmarks.md, llms-full.txt
Refreshes real-world examples, measurement tools, metrics, JSON serialization guidance, and related documentation links.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main documentation rewrite around the ShakaPerf twin-server benchmarking workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/rails-perf-benchmarks-docs-5450bd

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR refocuses the performance guide on proving improvements with paired ShakaPerf comparisons. The main changes are:

  • Adds the twin-server improve-and-prove workflow.
  • Reorganizes performance levers and benchmark interpretation guidance.
  • Updates renderer, hydration, caching, and JSON version guidance.
  • Regenerates the consolidated LLM documentation.

Confidence Score: 5/5

The documentation update looks mergeable after a small broken-link cleanup.

  • The implementation-backed configuration guidance is consistent with the inspected code.
  • Two remaining repository links now open missing page fragments.

docs/oss/core-concepts/performance-benchmarks.md

Important Files Changed

Filename Overview
docs/oss/core-concepts/performance-benchmarks.md Rewrites the guide around ShakaPerf and modernizes technical guidance, but removes two fragments still referenced by an internal planning document.
llms-full.txt Regenerates the consolidated documentation with the revised performance guide.

Reviews (1): Last reviewed commit: "docs: reframe performance benchmarks aro..." | Re-trigger Greptile

Comment thread docs/oss/core-concepts/performance-benchmarks.md

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ac811c0cd

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread docs/oss/core-concepts/performance-benchmarks.md
Comment thread docs/oss/core-concepts/performance-benchmarks.md Outdated
Comment thread docs/oss/core-concepts/performance-benchmarks.md Outdated
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review: docs/oss/core-concepts/performance-benchmarks.md rewrite

This is a docs-only PR (rewritten performance-benchmarks.md + regenerated llms-full.txt). I verified the factual claims against the codebase (Shakapacker default script-loading logic, Node Renderer default worker count, gemspec Ruby floor, anchor targets, and cross-doc links). Overall the rewrite is well-sourced and the staleness fixes it claims to make (selective-hydration default, ExecJS table, JSON gem Ruby floor) all check out against the current code:

  • configuration.rb: self.generated_component_packs_loading_strategy ||= (Utils.react_on_rails_pro? ? :async : :defer) — matches the corrected note that Pro on Shakapacker ≥ 8.2.0 already defaults to :async.
  • react_on_rails.gemspec / react_on_rails_pro.gemspec: required_ruby_version = ">= 3.3.0" — matches "Ruby ≥ 3.3" claim.
  • configBuilder.ts: os.cpus().length - 1 — matches "one per CPU minus one" Node Renderer worker default.
  • All 16 relative doc links and all anchors (#chunk-contamination, #level-1/2-*-caching, #live-demo-and-evidence, #benchmarking-rsc-against-warm-ssr-caches) resolve, and the stable anchor is genuinely referenced externally from rsc-performance-validation.md.

Two issues found:

  1. Broken GitHub link (inline comment on the diff): the in-repo example link uses tree/master/test/shakaperf/rsc-fouc, but this repo's default branch is main — every other GitHub source link in the docs uses tree/main/.... This link 404s.
  2. Cross-doc inconsistency (not in this PR's diff, so no inline comment possible): docs/oss/getting-started/oss-vs-pro.md:14 still says "3-10x faster SSR" — but this PR's own rationale for editing performance-benchmarks.md was to "stop asserting an unsourced 3-10x figure." The feature-matrix row now links to a page that explicitly disclaims that exact number ("the exact multiple is workload-dependent... rather than quoting a headline number"). Worth fixing in this PR or a fast follow so the two pages don't contradict each other.

No security or performance concerns — this is a documentation-only change.

coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
llms-full.txt (1)

5180-5180: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Bound the JSON dependency recommendation.

gem 'json', '>= 2.8' permits a future 3.x release with behavior changes. Use a tested 2.x constraint such as < 3, and distinguish the benchmarked version from the supported dependency range. Ruby’s documentation already notes behavior changes in JSON 3. (docs.ruby-lang.org)

🤖 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 `@llms-full.txt` at line 5180, Update the JSON gem dependency declaration to
constrain it to the tested 2.x series by adding an upper bound below version 3,
and distinguish the benchmarked version from the broader supported range.
🤖 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 `@llms-full.txt`:
- Around line 4910-4915: Update the report-output documentation to distinguish
the paths produced by compare from the default input path used by parse-report:
describe compare-results/full-report.html,
compare-results/self-contained-performance-report.html, and
compare-results/report.json, and state that parse-report defaults to
visreg_data/html_report/report.json or include the parse step that connects
these outputs.
- Around line 4939-4951: Update the ExecJS (mini_racer) baseline description in
the comparison table to note that mini_racer lacks TextEncoder, so React 18+ SSR
requires a polyfill or the Node Renderer. Avoid presenting unqualified
mini_racer as a valid modern React SSR baseline.

---

Nitpick comments:
In `@llms-full.txt`:
- Line 5180: Update the JSON gem dependency declaration to constrain it to the
tested 2.x series by adding an upper bound below version 3, and distinguish the
benchmarked version from the broader supported range.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 12804492-5747-45e5-8d4f-f966d911829d

📥 Commits

Reviewing files that changed from the base of the PR and between 2a65b6a and 3ac811c.

📒 Files selected for processing (2)
  • docs/oss/core-concepts/performance-benchmarks.md
  • llms-full.txt

Comment thread llms-full.txt Outdated
Comment thread llms-full.txt
…of loop

Rewrite core-concepts/performance-benchmarks.md so the page's spine is
"make a page measurably faster and prove the win" — including an AI agent
converting a page while a paired A/B benchmark keeps it honest — rather than
a lab-notebook of collected numbers.

Key changes:
- Add "The improve-and-prove loop" and "Proving a change with ShakaPerf":
  twin dockerized servers (control branch vs experiment branch), paired
  simultaneous sampling (Wilcoxon signed-rank + Hodges-Lehmann) that cancels
  laptop/CI noise so no quiet machine is needed, what it measures, how to run
  it, reports, and source-available licensing. ShakaPerf is public on npm and
  GitHub.
- Reorganize the tuning knobs into a ranked "performance levers" section, each
  linking its deep doc (Node renderer, RSC, streaming, caching, hydration
  scheduling, React Compiler, resource hints).
- Keep and retitle the warm-SSR-cache benchmarking guidance under a stable
  #benchmarking-rsc-against-warm-ssr-caches anchor (external docs link to it).

Staleness fixes flagged by audit:
- Correct the selective-hydration note: Pro apps on Shakapacker >= 8.2.0 already
  default to :async (was wrongly telling every reader to set it); :defer is the
  non-Pro default, :sync on Shakapacker < 8.2.0.
- Resolve the ExecJS table contradiction ("new process per eval" vs
  "Comparable") and stop asserting an unsourced 3-10x figure; anchor on the
  measured Popmenu 73% reduction and say the multiple is workload-dependent.
- Replace the ~150-line Gumroad section (all five tracking issues now closed,
  #3253/#3263 as NOT_PLANNED, superseded by the paired methodology) with a
  short honest pointer.
- Fix the JSON-gem Ruby floor: v17 requires Ruby >= 3.3; note Ruby 3.4+ ships
  json 2.9+ so the upgrade is unnecessary there.
- Expand cross-links (caching, hydration scheduling, profiling, web-vitals,
  bundle caching, React Compiler).

Regenerate llms-full.txt.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 21, 2026 09:38
@justin808
justin808 force-pushed the claude/rails-perf-benchmarks-docs-5450bd branch from 3ac811c to 988b155 Compare July 21, 2026 09:38
- Report output: pinned ShakaPerf 0.1.3 writes
  self-contained-performance-report.html, full-report.html, and report.json —
  not report.html (verified against internal/analysis/rsc-fouc-shakaperf-artifacts).
- In-repo example link: tree/master -> tree/main (repo default branch is main;
  no master branch, so the old link 404'd).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@justin808

Copy link
Copy Markdown
Member Author

Thanks — verified each point against the actual source. Summary of dispositions (fixes in cbdc1ae):

Fixed

  • Report filenames (@chatgpt-codex-connector, @coderabbitai): correct — the pinned ShakaPerf 0.1.3 output in internal/analysis/rsc-fouc-shakaperf-artifacts/reports/ is self-contained-performance-report.html, full-report.html, and report.json; there is no report.html. Updated the doc to name those exactly. (The report.html/report.json phrasing came from the 0.1.4 --skip-report help text, which doesn't match the pinned release.)
  • tree/mastertree/main (@claude): correct, the default branch is main with no master, so the link 404'd. Fixed.

Declined, with rationale

  • shaka-perf servers needs start-servers (@chatgpt-codex-connector): the authoritative README-twin-servers.md documents the no-subcommand servers form as "detect changes → rebuild → start containers → start servers via Overmind (blocks until Ctrl+C)" (README-twin-servers.md:100-116). The snippet already scopes itself to "after a one-time twin-servers setup" and links that guide for exact mechanics, so it is accurate as written.
  • Bound gem 'json', '>= 2.8' to < 3 (@coderabbitai): this is a performance recommendation in docs, not a shipped lockfile constraint; >= 2.8 matches this repo's own Gemfile.lock (json 2.19.8), no json 3.x exists yet, and an upper bound would steer users away from a future good release. Left as the standard idiom.
  • Dangling #gumroad-* anchors (@greptile-apps): the only remaining references are in internal/planning/3253-gumroad-staging-benchmark-plan.md, an internal planning doc for issue Follow-up: Capture Gumroad benchmark environment metadata #3253 — now closed as NOT_PLANNED. Not user-facing and superseded, so no compatibility alias added. No docs/ page links those anchors.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Comment thread docs/oss/core-concepts/performance-benchmarks.md
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@justin808 Thanks for the careful source verification and the clear dispositions.

The corrected ShakaPerf report filenames address my finding. Your rationale for retaining gem 'json', '>= 2.8' is also sound: this is forward-compatible documentation guidance, not a project dependency constraint, so an arbitrary < 3 cap would be inappropriate. The servers and retired internal-anchor explanations are noted as well.


✏️ Learnings added
Learnt from: justin808
URL: https://github.com/shakacode/react_on_rails/pull/4766

Timestamp: 2026-07-21T09:42:05.286Z
Learning: In React on Rails performance documentation, `gem 'json', '>= 2.8'` is a forward-compatible performance recommendation rather than a shipped dependency constraint; do not recommend adding an arbitrary `< 3` upper bound solely because a future major version may exist.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

All four declined items have solid rationale:

  • shaka-perf servers — the no-subcommand form is documented as authoritative in README-twin-servers.md; the doc accurately scopes to post-setup usage.
  • gem 'json', '>= 2.8' — matches the repo's own Gemfile.lock (json 2.19.8), no json 3.x exists to guard against, and an upper bound would penalize a future good release.
  • Dangling #gumroad-* anchors — only reference is in a closed/NOT_PLANNED internal planning doc, not user-facing. No alias needed.

And the two fixes in cbdc1ae are clean — correct report filenames from the pinned 0.1.3 artifacts and the dead tree/master link resolved. LGTM.

coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@docs/oss/core-concepts/performance-benchmarks.md`:
- Around line 106-107: Update the documented pValueThreshold default in the
abtests.config.ts configuration description from 0.05 to 0.01, matching the
referenced ShakaPerf documentation; leave the other defaults unchanged.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e28627c-df2a-4e0c-9cda-bec6dfbbb263

📥 Commits

Reviewing files that changed from the base of the PR and between 3ac811c and 988b155.

📒 Files selected for processing (2)
  • docs/oss/core-concepts/performance-benchmarks.md
  • llms-full.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • llms-full.txt

Comment thread docs/oss/core-concepts/performance-benchmarks.md
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Summary

This is a docs-only PR (docs/oss/core-concepts/performance-benchmarks.md + its generated llms-full.txt mirror) that reframes the performance benchmarks page around an "improve-and-prove" workflow backed by ShakaPerf. I fact-checked the substantive claims against the current codebase rather than just reading the prose.

What I verified

  • Selective hydration default fix is correct: react_on_rails/lib/react_on_rails/configuration.rb:180-184 confirms Pro + Shakapacker >= 8.2.0 defaults to :async; non-Pro defaults to :defer; pre-8.2.0 falls back to :sync with a warning. The old doc's instruction to manually set :async was indeed a no-op for Pro users on current Shakapacker and unsupported for non-Pro - good catch.
  • Node Renderer worker-count claim is accurate: packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts (defaultWorkersCount() = os.cpus().length - 1 || 1) and master.ts (cluster.fork()) match the new "one per CPU minus one" / cluster.fork() phrasing exactly.
  • ExecJS pool-size/JRuby claim is accurate, matches docs/oss/core-concepts/execjs-limitations.md:129-135.
  • Ruby >= 3.3 requirement for v17 is accurate, matches react_on_rails.gemspec:36 and react_on_rails_pro.gemspec:50 (required_ruby_version = ">= 3.3.0").
  • ShakaPerf config option names (numberOfMeasurements, regressionThreshold, pValueThreshold, samplingMode) and the PERF_EXPERIMENT env var match real usage in test/shakaperf/rsc-fouc/abtests.config.ts and .github/workflows/shakaperf-release-gates.yml.
  • test/shakaperf/rsc-fouc/ "release gate" claim is accurate per that directory's README.md.
  • All new/kept internal links and anchors resolve: docs/oss/migrating/rsc-performance-validation.md, rsc-troubleshooting.md#chunk-contamination, the building-features docs for hydration-scheduling/performance-tracks-and-profiling/web-vitals-and-rum/bundle-caching/react-compiler, docs/pro/react-server-components/critical-resource-hints.md, and docs/pro/react-server-components/index.md#live-demo-and-evidence all exist.
  • No dangling references to the removed anchors (gumroad-style-rsc-demo, gumroad-rsc-worst-case-responseend, gumroad-rsc-env-metadata-checklist, ssr-performance-execjs-vs-node-renderer) remain in docs/. One stale reference survives in internal/planning/3253-gumroad-staging-benchmark-plan.md (internal, not user-facing docs - FYI only, not a blocker).
  • The ExecJS table contradiction fix ("Comparable" vs. "New process per eval call" -> now "Slower (process spawn per eval)") resolves a genuine internal inconsistency in the old table.

Code quality / style

  • Left one inline nit: the ## JSON Serialization Performance heading wasn't normalized to sentence case like every other heading in this pass ("Real-world results", "Measuring your own performance", etc). Purely cosmetic - anchors are case-insensitive slugs so nothing breaks.
  • The new structure (improve-and-prove loop -> ShakaPerf harness -> levers -> honest-reading guidance -> real-world results) is a clear improvement over the old "lab notebook" structure, and removing ~150 lines of heavily-caveated, unpublished-artifact benchmark data in favor of a one-paragraph pointer is the right call given the tracking issues are closed.
  • llms-full.txt was regenerated in lockstep with the source doc - diffed both hunks and content matches.

Risks / things to watch

  • This page now leans heavily on external links (ShakaPerf's GitHub README, used_statistics.md, shakaperf.com pricing) for load-bearing claims (statistical methodology, licensing terms). None of that is verifiable from this repo, so if ShakaPerf's docs or pricing page change, this page can silently drift out of date. Not a blocker, just a maintenance note.
  • No functional/code risk - this PR touches only markdown docs and a generated llms-full.txt mirror, so there's no security or performance surface to review beyond content accuracy, which checks out.

Overall: well-researched, and every checkable factual claim I traced back to source code or existing docs held up. Nice work.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbdc1aec99

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread docs/oss/core-concepts/performance-benchmarks.md

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the React on Rails OSS performance benchmarking documentation to emphasize an “improve-and-prove” workflow (make a page faster, then verify the win) and to modernize guidance for the React 19.2 / RSC / v17 era, centering ShakaPerf’s paired A/B methodology.

Changes:

  • Reframes the page around an improve→convert→paired A/B proof loop, with ShakaPerf twin-server benchmarking as the primary recommended harness.
  • Refreshes and reorders key “performance levers” (Node renderer, RSC, streaming/selective hydration, caching, etc.) and preserves the warm-SSR-cache anchor.
  • Updates JSON serialization guidance and trims/repositions the older Gumroad-style benchmark content.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
docs/oss/core-concepts/performance-benchmarks.md Major rewrite: improve-and-prove workflow, ShakaPerf paired A/B guidance, updated levers and “reading results” sections.
llms-full.txt Regenerated aggregated doc content for the updated performance-benchmarks page.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/oss/core-concepts/performance-benchmarks.md
Copilot AI review requested due to automatic review settings July 21, 2026 09:44
Aligns the last remaining Title Case heading with the sentence-case style
applied to every other heading in this PR. Anchor #json-serialization-performance
is unchanged, so cross-links still resolve.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review: docs reframe of performance-benchmarks.md

Solid rewrite — spot-checked the concrete factual claims against the actual implementation and they all check out:

  • Selective hydration default (:async for Pro on Shakapacker >= 8.2.0, :defer non-Pro, :sync fallback) matches react_on_rails/lib/react_on_rails/configuration.rb:172-194 (validate_generated_component_packs_loading_strategy) exactly.
  • Node Renderer worker default (one per CPU minus one) matches defaultWorkersCount() (os.cpus().length - 1 || 1) in the Pro node-renderer's configBuilder.ts.
  • PERF_EXPERIMENT env var claim is corroborated by the twin-servers setup artifacts under internal/analysis/rsc-fouc-shakaperf-artifacts/.
  • Ruby >= 3.3 requirement matches react_on_rails/react_on_rails.gemspec:36 (required_ruby_version = '>= 3.3.0').
  • All new internal links resolve (hydration-scheduling.md, performance-tracks-and-profiling.md, web-vitals-and-rum.md, bundle-caching.md, react-compiler.md, critical-resource-hints.md, rsc-troubleshooting.md#chunk-contamination, test/shakaperf/rsc-fouc/), and the anchors that external docs depend on (#benchmarking-rsc-against-warm-ssr-caches, #popmenu, #public-marketplace-rsc-demo) are preserved.
  • llms-full.txt is a faithful mirror of the markdown change, no drift between the two.

One follow-up worth doing: internal/planning/3253-gumroad-staging-benchmark-plan.md links to two anchors this PR removes from performance-benchmarks.md — line 21: performance-benchmarks.md#gumroad-style-rsc-demo, and lines 43/118: performance-benchmarks.md#gumroad-rsc-env-metadata-checklist. Per the PR description, issue #3253 (which this planning doc exists to address) is now closed as NOT_PLANNED, so the doc itself looks stale independent of this PR. Since it's under internal/planning/ rather than docs/, it's understandable the PR's '16 relative links resolve' check didn't cover it, but worth a follow-up PR to either delete the plan doc or update its links now that the target anchors and the tracking issue are both gone.

Minor nit: deleting the Gumroad section entirely means anyone who bookmarked its old anchors from outside this repo will get a dead anchor. The PR description already confirms no external doc links to these, so this is a non-blocking observation, not a defect. No code changes are involved (pure docs), so there's no security or runtime-performance surface to review beyond the accuracy of the claims above.

Overall: well-verified, accurate, and a meaningful simplification (423 to 406 lines) while replacing an apologetic data dump with an actionable workflow. Approving modulo the stale-planning-doc follow-up.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 21, 2026 09:47
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, well-researched docs reframe. I verified the key factual claims against the actual source in this checkout, and they hold up:

  • Selective-hydration defaultgenerated_component_packs_loading_strategy default logic in react_on_rails/lib/react_on_rails/configuration.rb:180-184 confirms: Pro + Shakapacker ≥ 8.2.0 → :async, non-Pro → :defer, Shakapacker < 8.2.0 → :sync. Matches the new NOTE block exactly.
  • Node Renderer worker defaultdefaultWorkersCount() in packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts:151 is os.cpus().length - 1 || 1, matching the new "one per CPU minus one" phrasing (replacing the less precise cluster.fork() reference).
  • Ruby version claimreact_on_rails.gemspec:36 sets required_ruby_version = ">= 3.3.0", matching the new JSON-gem section's Ruby ≥ 3.3 framing.
  • All 17 relative doc links in the rewritten page resolve to real files in this checkout (spot-checked rsc-performance-validation.md, hydration-scheduling.md, bundle-caching.md, react-compiler.md, critical-resource-hints.md, rsc-troubleshooting.md, performance-tracks-and-profiling.md, web-vitals-and-rum.md, etc.), and the referenced anchors (#chunk-contamination, #level-1-prerender-caching, #level-2-fragment-caching, #benchmarking-rsc-against-warm-ssr-caches) all exist at those targets.
  • test/shakaperf/rsc-fouc/ (the in-repo ShakaPerf example cited) exists with the described config.

Issue found: stale internal doc now has dead anchor links

internal/planning/3253-gumroad-staging-benchmark-plan.md links to performance-benchmarks.md#gumroad-style-rsc-demo (line 21) and performance-benchmarks.md#gumroad-rsc-env-metadata-checklist (lines 43, 118) — both anchors are removed by this PR along with the Gumroad section they belonged to. Since the PR description says issue #3253 (which this planning doc exists to address) is now closed as NOT_PLANNED, this file appears to be an orphaned artifact from before that decision. Worth either updating its links or removing/archiving the file in this PR (or a fast follow) so it doesn't sit with broken internal links. This is outside docs/ proper so it wouldn't be caught by script/check-docs-sidebar or an external-link check, which is presumably why the PR's "16 relative links resolve" verification didn't surface it — that count is scoped to the page's own outbound links, not inbound references to it from elsewhere in the repo.

Minor observations (non-blocking)

  • The new "Relative speed" row in the ExecJS-vs-Node-Renderer table ("Slower (process spawn per eval)" / "Substantially faster") replaces the old unsourced "3-10x" figure with qualitative language — good call given the PR's own stated goal of not asserting unsourced multipliers, and it's consistent with the Popmenu-anchored framing used elsewhere on the page.
  • llms-full.txt's diff mirrors the .md diff hunk-for-hunk in this PR, consistent with the PR description's claim that it was regenerated via the generator script rather than hand-edited separately — reduces the risk of the two drifting.
  • No code, config, or test changes — this is a docs-only PR (2 files: the page itself and its generated llms-full.txt mirror), so there's no security or runtime-performance surface to review beyond content accuracy, which checks out.

Overall: the factual corrections are accurate and well-sourced, the restructuring is a clear improvement over the caveat-heavy previous version, and internal links within the page are sound. The one actionable item is the dead-link cleanup in the internal/planning/ file noted above.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread docs/oss/core-concepts/performance-benchmarks.md
Comment thread docs/oss/core-concepts/performance-benchmarks.md
Comment thread docs/oss/core-concepts/performance-benchmarks.md
@justin808
justin808 dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot] July 21, 2026 22:39

Addressed in cbdc1ae/f6d3261: fixed report filenames and tree/master to tree/main; remaining points verified against source and declined with rationale (config-file pValueThreshold default is 0.05; json >=2.8 matches repo Gemfile.lock); all review threads resolved. Dismissing to enqueue per maintainer.

@justin808
justin808 added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit 11dae98 Jul 21, 2026
43 checks passed
@justin808
justin808 deleted the claude/rails-perf-benchmarks-docs-5450bd branch July 21, 2026 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants