ci(benchmark/react): add benchmark for various attribute update#1650
ci(benchmark/react): add benchmark for various attribute update#1650hzy merged 1 commit intolynx-family:mainfrom
Conversation
|
📝 WalkthroughWalkthroughAdds a new React benchmark case (004-various-update) that instruments SnapshotInstance.setAttribute for named keys to record Codspeed benchmarks, renders a component exercising various attribute updates and spreads, and integrates the case into build/config via lynx.config.js, package scripts, and exports new helpers (PREFIX, isMainThread) in hook.ts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Web Explorer#4825 Bundle Size — 367.43KiB (0%).dcb2fcd(current) vs f454292 main#4823(baseline) Bundle metrics
|
| Current #4825 |
Baseline #4823 |
|
|---|---|---|
 Initial JS |
144.23KiB |
144.23KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
219(+0.46%) |
218 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.33% |
3.33% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4825 |
Baseline #4823 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch hzy:p/hzy/bench_5 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4834 Bundle Size — 237.5KiB (0%).dcb2fcd(current) vs f454292 main#4832(baseline) Bundle metrics
|
| Current #4834 |
Baseline #4832 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
160 |
160 |
| Duplicate Modules |
65 |
65 |
| Duplicate Code |
45.83% |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4834 |
Baseline #4832 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.74KiB |
91.74KiB |
Bundle analysis report Branch hzy:p/hzy/bench_5 Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1650 will not alter performanceComparing Summary
Benchmarks breakdown
|
hzy
force-pushed
the
p/hzy/bench_5
branch
3 times, most recently
from
aef6f47 to
f790d5d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds benchmark infrastructure for ReactLynx, including benchmarks for list rendering and attribute updates. The changes enhance the benchmarking capabilities to measure performance of different ReactLynx operations.
Key Changes
- Added benchmarks for list operations and attribute updates
- Refactored existing benchmark code by extracting reusable components
- Enhanced build preparation scripts to avoid unnecessary rebuilds
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/lynx/benchx_cli/scripts/prepare.mjs | Added early exit optimization when build artifacts already exist |
| packages/lynx/benchx_cli/scripts/build.mjs | Updated commit hash reference for build process |
| benchmark/react/src/patchUpdateListCallbacks.ts | New file implementing Codspeed benchmarks for list callback operations |
| benchmark/react/src/patchProfile.ts | Updated to use shared PREFIX constant from hook module |
| benchmark/react/src/hook.ts | Added shared constants and utilities for benchmarking |
| benchmark/react/package.json | Added new benchmark scripts and dependencies for list and attribute update tests |
| benchmark/react/lynx.config.js | Added entry points for new benchmark cases |
| benchmark/react/cases/004-various-update/index.tsx | New benchmark testing various attribute update operations |
| benchmark/react/cases/003-hello-list/index.tsx | New benchmark testing list rendering and scrolling operations |
| benchmark/react/cases/002-hello-reactLynx/index.tsx | Refactored to use extracted RecursiveText component |
| benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx | Extracted reusable RecursiveText component from main benchmark |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (17)
benchmark/react/package.json (2)
10-11: Bench scripts should build before running to avoid missing dist bundlesAdd a prebench step so CI/dev never runs against stale/missing bundles.
"scripts": { + "prebench": "pnpm build", "bench": "pnpm run --sequential --stream --aggregate-output '/^bench:.*/'",
16-19: Perfetto runs should also build firstMirror the bench change for perfetto to ensure bundles exist.
"scripts": { + "preperfetto": "pnpm build", "perfetto": "pnpm run --sequential --stream --aggregate-output '/^perfetto:.*/'",packages/lynx/benchx_cli/scripts/build.mjs (1)
100-102: Optional: make cherry-pick fetch more robustFetching a bare SHA can be flaky across mirrors. Consider fetching to FETCH_HEAD explicitly and cherry-picking that.
-git fetch hzy ${PICK_COMMIT} -git cherry-pick -n ${PICK_COMMIT} +git fetch hzy ${PICK_COMMIT} +git cherry-pick -n FETCH_HEADbenchmark/react/src/hook.ts (1)
18-22: Avoid // @ts-expect-error and normalize path separatorsDeclare ambient globals instead of suppressing, and normalize backslashes for safety.
+declare const __REPO_FILEPATH__: string; +declare const __CreatePage: unknown; -export const PREFIX = __REPO_FILEPATH__.split('/').slice(0, -2).join('/'); +export const PREFIX = __REPO_FILEPATH__.replace(/\\/g, '/').split('/').slice(0, -2).join('/'); - -export const isMainThread = - // @ts-expect-error safely check - typeof __CreatePage === 'function'; +export const isMainThread = typeof __CreatePage === 'function';packages/lynx/benchx_cli/scripts/prepare.mjs (1)
15-26: Ensure executable bit on non-Windows stubsSet +x to avoid edge cases where copied file loses exec perms in some filesystems.
-import { copyFileSync, existsSync, mkdirSync, writeFileSync } from 'node:fs'; +import { copyFileSync, existsSync, mkdirSync, writeFileSync, chmodSync } from 'node:fs'; @@ } else { copyFileSync('/usr/bin/true', './dist/bin/benchx_cli'); + try { chmodSync('./dist/bin/benchx_cli', 0o755); } catch {} }benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx (1)
5-18: Avoid O(n^2) string joins in recursion; keep nested structureJoining the remainder each step reallocates the string repeatedly. Use index-based recursion over a precomputed char array; same DOM shape, better perf.
-function RecursiveText(props: { text: string }) { - const { text } = props; - const sliced = [...text]; - const [first, ...rest] = sliced; - - return ( - sliced.length > 0 && ( - <text> - {first} - <RecursiveText text={rest.join('')} /> - </text> - ) - ); -} +function RecursiveText(props: { text: string }) { + const chars = [...props.text]; + function R(i: number): JSX.Element | null { + if (i >= chars.length) return null; + return ( + <text> + {chars[i]} + {R(i + 1)} + </text> + ); + } + return R(0); +}benchmark/react/lynx.config.js (3)
35-42: Ensure 004-various-update is instrumented if you want Codspeed stats.003 wires patchProfile.ts (and list-callback patch). 004 currently doesn’t include patchProfile.ts, so Codspeed.setExecutedBenchmark from patchProfile won’t run for that entry.
If 004 should be profiled similarly, add patchProfile:
'004-various-update': [ - './cases/004-various-update/index.tsx', + './src/patchProfile.ts', + './cases/004-various-update/index.tsx', ],
12-23: Minify flags are contradictory.output.minify.js is true but jsOptions.minify is false. This is confusing and may lead to unexpected output.
Consider dropping one to reflect intended behavior.
24-43: Changeset reminder for CI.Per prior repo learnings, CI may require a changeset when “src/**” is touched—even for internal tooling—often satisfied via an empty changeset file. This PR adds files under benchmark/react/src.
I used your stored learnings. If CI complains, I can generate a minimal empty changeset entry.
benchmark/react/src/patchProfile.ts (1)
36-41: Gate noisy console logs.The nested-benchmark console.log can spam CI logs. Consider guarding behind a debug flag.
- console.log( + if (process.env.LYNX_BENCH_DEBUG) console.log(benchmark/react/cases/003-hello-list/index.tsx (2)
16-35: Harden scroll invoke with error/complete handling.If the first invoke fails, we’ll never flip stopBenchmark. Add fail/complete handlers to keep the run bounded.
- listRef.current!.invoke({ + listRef.current!.invoke({ method: 'scrollToPosition', params: { position: 1, smooth: false }, - success: function() { + success: function() { listRef.current!.invoke({ method: 'scrollToPosition', params: { position: 2, smooth: false }, - success: function() { + success: function() { setStopBenchmark(true); }, + fail: function() { setStopBenchmark(true); }, + complete: function() { /* optional: metrics */ }, }).exec(); }, + fail: function() { setStopBenchmark(true); }, + complete: function() { /* optional: metrics */ }, }).exec();Confirm the exact callback names (e.g., fail/complete) for NodesRef.invoke in your runtime; adjust accordingly.
48-66: Add React keys to mapped children to avoid warnings.item-key is consumed by Lynx, but React still benefits from a key prop for reconciliation.
- <list-item + <list-item + key={index} item-key={`${index}`}benchmark/react/src/patchUpdateListCallbacks.ts (1)
41-58: Guard against missing componentAtIndex when hooking.Be defensive if componentAtIndex is undefined/null.
- old!( + old!( list, - withCodspeed(componentAtIndex), + componentAtIndex ? withCodspeed(componentAtIndex) : componentAtIndex, enqueueComponent, componentAtIndexes, );benchmark/react/cases/004-various-update/index.tsx (4)
9-15: Make the patch idempotent to prevent double-hooking under HMR/re-imports.Guard the prototype patch with a Symbol flag.
Apply this diff:
-import { hook, isMainThread } from '../../src/hook.js'; +import { hook, isMainThread } from '../../src/hook.js'; -if (__MAIN_THREAD__) { +const PATCH_FLAG = Symbol.for('lynx.bench.setAttribute.patched'); +if (__MAIN_THREAD__ && !(PATCH_FLAG in SnapshotInstance.prototype)) { + Object.defineProperty(SnapshotInstance.prototype, PATCH_FLAG, { value: true }); hook( SnapshotInstance.prototype, 'setAttribute', function(this: SnapshotInstance, old, key, value) {
21-68: Replace the long switch with a fast lookup to reduce overhead in the hot path.Keeps the hook lightweight and easier to maintain.
Apply this diff (includes a small table and the in-function replacement):
@@ -import { hook, isMainThread } from '../../src/hook.js'; +import { hook, isMainThread } from '../../src/hook.js'; + +const KEY_NAMES = [ + 'Attr', // 0 + 'Dataset', // 1 + 'Event', // 2 + 'MT_Event', // 3 + 'StyleString', // 4 + 'StyleObject', // 5 + 'Class', // 6 + 'Id', // 7 + 'Ref', // 8 + 'TimingFlag', // 9 + 'MT_Ref', // 10 + 'ListItemPlatformInfo', // 11 + 'ListItemPlatformInfoSpread', // 12 + 'Spread', // 13 +] as const; @@ - let name = ''; - switch (key) { - case 0: - name = 'Attr'; - break; - case 1: - name = 'Dataset'; - break; - case 2: - name = 'Event'; - break; - case 3: - name = 'MT_Event'; - break; - case 4: - name = 'StyleString'; - break; - case 5: - name = 'StyleObject'; - break; - case 6: - name = 'Class'; - break; - case 7: - name = 'Id'; - break; - case 8: - name = 'Ref'; - break; - case 9: - name = 'TimingFlag'; - break; - case 10: - name = 'MT_Ref'; - break; - case 11: - name = 'ListItemPlatformInfo'; - break; - case 12: - name = 'ListItemPlatformInfoSpread'; - break; - case 13: - name = 'Spread'; - break; - case 'values': - name = 'BatchedValues'; - break; - } + const name = + typeof key === 'number' + ? (KEY_NAMES[key as number] ?? '') + : (key === 'values' ? 'BatchedValues' : '');
114-138: Preserve the tuple type on setValues to avoid accidental widening.Minor type-safety improvement; no runtime impact.
Apply this diff:
- setValues([ + setValues([ 'some-other-exposure-id', 'some-other-dataset', () => {}, () => { 'main thread'; }, 'width: 200rpx; height: 100rpx; background-color: #FACE00;', { width: '200rpx', height: '100rpx', backgroundColor: '#FACE00', }, 'some-other-css-class', 'some-other-id', (_e: NodesRef) => {}, 'some_other_lynx_timing_flag', (_e: MainThread.Element) => { 'main thread'; }, 'some-other-item-key', - ]); + ] as const);
11-85: Optional: avoid double-counting nested calls (e.g., when key === 'values').If you want only the outer “BatchedValues” to be timed/recorded, gate start/stop with a simple depth counter.
Example patch (conceptual; integrate with the earlier try/finally):
+let benchDepth = 0; @@ - Codspeed.startBenchmark(); + const outer = benchDepth++ === 0; + if (outer) Codspeed.startBenchmark(); try { return old!.call(this, key, value); } finally { - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, - ); + if (--benchDepth === 0) { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, + ); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx(1 hunks)benchmark/react/cases/002-hello-reactLynx/index.tsx(1 hunks)benchmark/react/cases/003-hello-list/index.tsx(1 hunks)benchmark/react/cases/004-various-update/index.tsx(1 hunks)benchmark/react/lynx.config.js(1 hunks)benchmark/react/package.json(2 hunks)benchmark/react/src/hook.ts(1 hunks)benchmark/react/src/patchProfile.ts(1 hunks)benchmark/react/src/patchUpdateListCallbacks.ts(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(1 hunks)packages/lynx/benchx_cli/scripts/prepare.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
benchmark/react/src/patchUpdateListCallbacks.ts
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
benchmark/react/lynx.config.js
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
benchmark/react/lynx.config.js
📚 Learning: 2025-08-27T08:10:09.932Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1612
File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13
Timestamp: 2025-08-27T08:10:09.932Z
Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.
Applied to files:
benchmark/react/package.json
🧬 Code graph analysis (2)
benchmark/react/src/patchUpdateListCallbacks.ts (3)
packages/web-platform/web-constants/src/types/Element.ts (1)
ComponentAtIndexCallback(22-28)packages/web-platform/web-mainthread-apis/src/pureElementPAPIs.ts (1)
__GetChildren(73-75)benchmark/react/src/hook.ts (2)
PREFIX(18-18)hook(5-16)
benchmark/react/cases/004-various-update/index.tsx (1)
benchmark/react/src/hook.ts (2)
hook(5-16)isMainThread(20-22)
🔇 Additional comments (9)
benchmark/react/package.json (1)
33-33: No changes needed: @lynx-js/type-element-api is only imported viaimport type(erased at compile time) and isn’t required at runtime, so leaving it as a devDependency is appropriate.packages/lynx/benchx_cli/scripts/prepare.mjs (1)
5-13: Good idempotency guardEarly-exit on existing binary prevents redundant work in CI/local.
benchmark/react/cases/002-hello-reactLynx/index.tsx (1)
7-13: Good extraction to a reusable RecursiveText component.This removes duplication and keeps 002 aligned with 003’s usage.
benchmark/react/src/patchProfile.ts (1)
5-5: Good: centralizing PREFIX import.Using the shared PREFIX from hook.js keeps labels consistent across patches.
benchmark/react/src/patchUpdateListCallbacks.ts (2)
10-10: No action needed:__MAIN_THREAD__is already declared globally.
The ambient declaration exists in packages/react/runtime/types/types.d.ts (line 20).
5-6: ConfirmComponentAtIndexCallbackreturnsvoidin both upstream definitions
Both local definitions inweb-constantsand React’s runtime types declare the callback as returningvoid, so your import from@lynx-js/type-element-apialigns with the upstream signature and won’t produce unexpectedundefinedat runtime.benchmark/react/cases/004-various-update/index.tsx (3)
140-142: Main-thread short-circuit is correct for this bench.Avoids rendering on the patched thread; matches the hook placement.
166-176: Good coverage of spread vs. explicit props for list-item/view.This should exercise keys 11/12 and 13 distinctly.
Also applies to: 178-195
1-205: Ensure global macros exist and hook is unique.
- Ambient typings in
benchmark/**/*.d.tsdeclareCodspeedand__REPO_FILEPATH__, but not__MAIN_THREAD__or__webpack_chunkname__—confirm your build tooling injects those.- Only one
hook(SnapshotInstance.prototype, 'setAttribute', …)call is present underbenchmark/**(no duplicates).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
benchmark/react/src/hook.ts (2)
20-22: Avoid @ts-expect-error by using globalThis for the probe.
Removes brittle expect-error while keeping behavior.-export const isMainThread = - // @ts-expect-error safely check - typeof __CreatePage === 'function'; +export const isMainThread = + typeof (globalThis as any).__CreatePage === 'function';
18-18: Normalize and type-assert REPO_FILEPATH in src/hook.ts
Insrc/hook.ts, update to:-export const PREFIX = __REPO_FILEPATH__.split('/').slice(0, -2).join('/'); +export const PREFIX = (__REPO_FILEPATH__ as string) + .replace(/\\/g, '/') + .split('/') + .slice(0, -2) + .join('/');Global
__REPO_FILEPATH__is already declared inbenchmark/react/rspeedy-env.d.ts, so no new ambient declaration is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
benchmark/react/cases/004-various-update/index.tsx(1 hunks)benchmark/react/lynx.config.js(1 hunks)benchmark/react/package.json(1 hunks)benchmark/react/src/hook.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmark/react/lynx.config.js
- benchmark/react/cases/004-various-update/index.tsx
🔇 Additional comments (1)
benchmark/react/package.json (1)
18-18: Perfetto script aligns with prior cases.
Consistent naming and flags.
renderComponentbefore hydrate (fix(react): run all pendingrenderComponentbefore hydrate #1438)Summary by CodeRabbit
New Features
Chores
Checklist