Skip to content

feat: add a trace for renderMainThread#2398

Closed
HuJean wants to merge 1 commit intomainfrom
p/perf-trace
Closed

feat: add a trace for renderMainThread#2398
HuJean wants to merge 1 commit intomainfrom
p/perf-trace

Conversation

@HuJean
Copy link
Copy Markdown
Collaborator

@HuJean HuJean commented Mar 30, 2026

Summary by CodeRabbit

  • Chores
    • Added a changeset metadata file.
    • Improved internal profiling instrumentation to better track rendering and rendering-stage performance.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).
  • Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).

@HuJean HuJean requested review from Yradex and hzy as code owners March 30, 2026 07:15
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 30, 2026

🦋 Changeset detected

Latest commit: cf3d912

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ea980a4f-da78-4c7d-ba9c-223e0535c55e

📥 Commits

Reviewing files that changed from the base of the PR and between 9b0c38b and cf3d912.

📒 Files selected for processing (2)
  • .changeset/itchy-dogs-search.md
  • packages/react/runtime/src/lifecycle/render.ts
✅ Files skipped from review due to trivial changes (1)
  • .changeset/itchy-dogs-search.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/react/runtime/src/lifecycle/render.ts

📝 Walkthrough

Walkthrough

Added an empty Changeset markdown file and modified renderMainThread() in the React runtime to add conditional profiling start/end calls, resulting in a duplicated conditional profileEnd() in addition to the existing profileEnd() in the finally block.

Changes

Cohort / File(s) Summary
Changesets
.changeset/itchy-dogs-search.md
Added a new Changeset markdown file containing only YAML frontmatter markers (--- ... ---) with no version or content fields.
React Runtime Profiling
packages/react/runtime/src/lifecycle/render.ts
Inserted a conditional profileStart('ReactLynx::renderMainThread') before the try, changed an inner profileStart tag to ReactLynx::renderToString, and added an extra conditional profileEnd() after the render pipeline, duplicating the existing profileEnd() in the finally.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • Yradex
  • hzy

Poem

🐰 I hop in code, a curious sight,

I mark the starts and ends just right,
Two little ends now join the run,
Timing dances under sun,
Hooray — the render race is fun! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add a trace for renderMainThread' directly matches the main objective of the PR and accurately describes the primary change to the renderMainThread function.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch p/perf-trace

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 and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@packages/react/runtime/src/lifecycle/render.ts`:
- Around line 18-20: The duplicate profiling span should be removed and the
existing ReactLynx::renderMainThread trace (created via profileStart/profileEnd)
should be expanded to cover the entire renderMainThread function; move the
single profileStart to the outermost entry of renderMainThread and wrap the
function body in a try/finally that always calls profileEnd, removing the inner
profileStart/profileEnd pair that currently encloses renderOpcodesInto() and the
test-only opcode remap so the trace is not nested or left unbalanced if remap or
renderOpcodesInto() throws.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 53087848-02b1-40a7-be07-67e2acc6b84e

📥 Commits

Reviewing files that changed from the base of the PR and between 78493b4 and 9b0c38b.

📒 Files selected for processing (2)
  • .changeset/itchy-dogs-search.md
  • packages/react/runtime/src/lifecycle/render.ts

Comment on lines +18 to +20
if (typeof __PROFILE__ !== 'undefined' && __PROFILE__) {
profileStart('ReactLynx::renderMainThread');
}
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.

⚠️ Potential issue | 🟠 Major

Expand the existing renderMainThread trace instead of adding a second one.

Lines 23-33 already manage a ReactLynx::renderMainThread span. Adding another start here and another end on Line 58 nests the same trace name, and the new outer end is skipped if the test-only opcode remap or renderOpcodesInto() throws. Since profileEnd just forwards to the native profiler, that can leave profiling state unbalanced and pollute later measurements. If the intent is to cover the whole function, widen the current trace to a single outer try/finally and drop the inner duplicate.

Also applies to: 57-59

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/react/runtime/src/lifecycle/render.ts` around lines 18 - 20, The
duplicate profiling span should be removed and the existing
ReactLynx::renderMainThread trace (created via profileStart/profileEnd) should
be expanded to cover the entire renderMainThread function; move the single
profileStart to the outermost entry of renderMainThread and wrap the function
body in a try/finally that always calls profileEnd, removing the inner
profileStart/profileEnd pair that currently encloses renderOpcodesInto() and the
test-only opcode remap so the trace is not nested or left unbalanced if remap or
renderOpcodesInto() throws.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 30, 2026

Merging this PR will degrade performance by 66.25%

⚡ 1 improved benchmark
❌ 5 regressed benchmarks
✅ 62 untouched benchmarks
⏩ 25 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
006-static-raw-text__main-thread-renderMainThread 1.8 ms 5.3 ms -66.25%
003-hello-list__main-thread-renderMainThread 16.6 ms 19.1 ms -12.92%
002-hello-reactLynx__main-thread-renderMainThread 2.2 ms 4 ms -44.97%
002-hello-reactLynx-destroyBackground 861.5 µs 680.1 µs +26.69%
007-four-layer-views__main-thread-renderMainThread 185.1 ms 402.8 ms -54.05%
transform 1000 view elements 40.4 ms 47.3 ms -14.74%

Comparing p/perf-trace (cf3d912) with main (78493b4)

Open in CodSpeed

Footnotes

  1. 25 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@relativeci
Copy link
Copy Markdown

relativeci Bot commented Mar 30, 2026

React MTF Example

#89 Bundle Size — 207.47KiB (0%).

cf3d912(current) vs 78493b4 main#76(baseline)

Bundle metrics  no changes
                 Current
#89
     Baseline
#76
No change  Initial JS 0B 0B
No change  Initial CSS 0B 0B
No change  Cache Invalidation 0% 0%
No change  Chunks 0 0
No change  Assets 3 3
No change  Modules 174 174
No change  Duplicate Modules 68 68
No change  Duplicate Code 46.09% 46.09%
No change  Packages 2 2
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#89
     Baseline
#76
No change  IMG 111.23KiB 111.23KiB
No change  Other 96.24KiB 96.24KiB

Bundle analysis reportBranch p/perf-traceProject dashboard


Generated by RelativeCIDocumentationReport issue

@relativeci
Copy link
Copy Markdown

relativeci Bot commented Mar 30, 2026

React Example

#6955 Bundle Size — 237.89KiB (0%).

cf3d912(current) vs 78493b4 main#6942(baseline)

Bundle metrics  no changes
                 Current
#6955
     Baseline
#6942
No change  Initial JS 0B 0B
No change  Initial CSS 0B 0B
No change  Cache Invalidation 0% 0%
No change  Chunks 0 0
No change  Assets 4 4
No change  Modules 180 180
No change  Duplicate Modules 71 71
No change  Duplicate Code 46.4% 46.4%
No change  Packages 2 2
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#6955
     Baseline
#6942
No change  IMG 145.76KiB 145.76KiB
No change  Other 92.13KiB 92.13KiB

Bundle analysis reportBranch p/perf-traceProject dashboard


Generated by RelativeCIDocumentationReport issue

@relativeci
Copy link
Copy Markdown

relativeci Bot commented Mar 30, 2026

Web Explorer

#8533 Bundle Size — 728.6KiB (0%).

cf3d912(current) vs 78493b4 main#8520(baseline)

Bundle metrics  Change 1 change
                 Current
#8533
     Baseline
#8520
No change  Initial JS 43.3KiB 43.3KiB
No change  Initial CSS 2.16KiB 2.16KiB
No change  Cache Invalidation 0% 0%
No change  Chunks 8 8
No change  Assets 10 10
No change  Modules 148 148
No change  Duplicate Modules 11 11
Change  Duplicate Code 34.69%(-0.03%) 34.7%
No change  Packages 3 3
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#8533
     Baseline
#8520
No change  Other 384.4KiB 384.4KiB
No change  JS 342.04KiB 342.04KiB
No change  CSS 2.16KiB 2.16KiB

Bundle analysis reportBranch p/perf-traceProject dashboard


Generated by RelativeCIDocumentationReport issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant