ci(benchmark/react): enable list in benchmark#1635
Conversation
|
📝 WalkthroughWalkthroughAdds a new RecursiveText component and refactors case 002 to use it. Introduces a new benchmark case 003 (hello-list), associated instrumentation (patchUpdateListCallbacks), config entries, and scripts. Centralizes PREFIX in hook.ts and updates patchProfile.ts to import it. Updates benchx_cli build commit and adds an idempotent prepare guard. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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! |
React Example#4820 Bundle Size — 237.5KiB (0%).3ec2001(current) vs af2d59b main#4814(baseline) Bundle metrics
|
| Current #4820 |
Baseline #4814 |
|
|---|---|---|
 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 #4820 |
Baseline #4814 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.74KiB |
91.74KiB |
Bundle analysis report Branch hzy:p/hzy/bench_4 Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#4811 Bundle Size — 367.43KiB (0%).3ec2001(current) vs af2d59b main#4805(baseline) Bundle metrics
|
| Current #4811 |
Baseline #4805 |
|
|---|---|---|
| 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.45%) |
220 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.33%(+0.3%) |
3.32% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4811 |
Baseline #4805 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch hzy:p/hzy/bench_4 Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1635 will improve performances by 10.88%Comparing 🎉 Hooray!
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| 🆕 | 003-hello-list-destroyBackground |
N/A | 2.5 ms | N/A |
| 🆕 | 003-hello-list-hydrate |
N/A | 8.3 ms | N/A |
| 🆕 | 003-hello-list-renderBackground |
N/A | 18.5 ms | N/A |
| 🆕 | 003-hello-list__main-thread-componentAtIndex__create |
N/A | 5.2 ms | N/A |
| 🆕 | 003-hello-list__main-thread-componentAtIndex__reuse |
N/A | 3.1 ms | N/A |
| 🆕 | 003-hello-list__main-thread-processData |
N/A | 22.5 µs | N/A |
| 🆕 | 003-hello-list__main-thread-renderMainThread |
N/A | 15.2 ms | N/A |
| 🆕 | 003-hello-list__main-thread-renderOpcodes |
N/A | 2.2 ms | N/A |
| 🆕 | 003-hello-list__main-thread-serializeRoot |
N/A | 3.8 ms | N/A |
| ⚡ | basic-performance-nest-level-100 |
6.7 ms | 6 ms | +10.88% |
hzy
force-pushed
the
p/hzy/bench_4
branch
6 times, most recently
from
3fbb09b to
415f2ea
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enables list benchmarking in the React benchmark suite by adding a new "hello-list" benchmark test case that measures list component performance with scrolling operations.
Key changes:
- Creates a new list benchmark case with scroll operations and component reuse tracking
- Extracts and shares the RecursiveText component between benchmark cases
- Adds Codspeed integration for list-specific performance measurements
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/lynx/benchx_cli/scripts/build.mjs |
Updates commit hash for build script |
benchmark/react/src/patchUpdateListCallbacks.ts |
Adds Codspeed integration for list component benchmarking |
benchmark/react/src/patchProfile.ts |
Refactors to use shared PREFIX constant |
benchmark/react/src/hook.ts |
Exports shared PREFIX constant for benchmark naming |
benchmark/react/package.json |
Adds scripts for list benchmark and perfetto tracing |
benchmark/react/lynx.config.js |
Configures build entry for hello-list benchmark |
benchmark/react/cases/003-hello-list/index.tsx |
Implements new list benchmark with scroll operations |
benchmark/react/cases/002-hello-reactLynx/index.tsx |
Refactors to use extracted RecursiveText component |
benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx |
Extracts RecursiveText component for reuse |
![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
🧹 Nitpick comments (9)
benchmark/react/src/hook.ts (1)
18-18: Normalize PREFIX splitting for cross-platform pathsVerified
__REPO_FILEPATH__is declared in benchmark/react/rspeedy-env.d.ts:59. Apply:-export const PREFIX = __REPO_FILEPATH__.split('/').slice(0, -2).join('/'); +export const PREFIX = __REPO_FILEPATH__.split(/[/\\]/).slice(0, -2).join('/');benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx (2)
10-17: Return null explicitly instead of boolean falseBe explicit about the base case to avoid returning a bare boolean.
- return ( - sliced.length > 0 && ( - <text> - {first} - <RecursiveText text={rest.join('')} /> - </text> - ) - ); + if (sliced.length === 0) return null; + return ( + <text> + {first} + <RecursiveText text={rest.join('')} /> + </text> + );
5-18: Avoid O(n^2) string joins in recursion (optional, benchmark-sensitive)Joining on every frame makes total work quadratic in input length. If this case ever uses longer strings, consider an index-based approach.
Example alternative:
function RecursiveText(props: { text: string }) { const arr = [...props.text]; function NodeAt(i: number): JSX.Element | null { if (i >= arr.length) return null; return <text>{arr[i]}<NodeAt i={i + 1} /></text>; } return NodeAt(0); }benchmark/react/package.json (1)
17-17: Consider a minimal smoke test instead of disabling testsEchoing “No tests specified” can mask regressions in CI. A tiny render smoke test would give basic guardrails without adding overhead.
Example:
{ "scripts": { "test": "node -e \"console.log('benchmark-react smoke: OK')\"" } }benchmark/react/rspeedy-env.d.ts (2)
14-16: Prefer importing official types when availableOnce @lynx-js/type-element-api (or equivalent) exports these, replace local aliases to avoid drift.
59-59: Consider declaring webpack_chunkname for type safetyUsed elsewhere but not declared here.
declare const __REPO_FILEPATH__: string; +declare const __webpack_chunkname__: string;benchmark/react/src/patchProfile.ts (1)
5-5: Verify .js extension resolution for TS importIf moduleResolution isn’t “Bundler”, this may fail. Prefer extensionless import for TS.
-import { PREFIX, hook } from './hook.js'; +import { PREFIX, hook } from './hook';Optionally add
import '@lynx-js/react';to ensurelynx.performanceexists before hooking across bundlers.benchmark/react/cases/003-hello-list/index.tsx (2)
16-35: Guard ref usage or assert lifecycle guaranteesNon-null assertion is fine if Lynx guarantees ref readiness before useEffect. If not guaranteed, use optional chaining to avoid crashes.
- listRef.current!.invoke({ + listRef.current?.invoke?.({
51-58: Add React key on list itemsEven with item-key for Lynx, React reconciliation benefits from a key.
- <list-item + <list-item + key={index} item-key={`${index}`}
📜 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 (10)
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/lynx.config.js(1 hunks)benchmark/react/package.json(1 hunks)benchmark/react/rspeedy-env.d.ts(1 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)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 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/rspeedy-env.d.ts (2)
packages/web-platform/web-constants/src/types/Element.ts (2)
ComponentAtIndexCallback(22-28)EnqueueComponentCallback(30-34)packages/web-platform/web-mainthread-apis/src/pureElementPAPIs.ts (1)
__GetChildren(73-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: CodeQL Analyze (javascript-typescript)
- GitHub Check: CodeQL Analyze (actions)
🔇 Additional comments (8)
packages/lynx/benchx_cli/scripts/build.mjs (1)
33-33: Verified PICK_COMMIT exists upstream – commit 78493ae8581046275f6333ce16004666560ce058 is present and fetchable by CI.benchmark/react/cases/002-hello-reactLynx/index.tsx (2)
12-12: LGTM: component extraction improves readability and reuseSwitching to clarifies intent and keeps the case file lean.
7-7: No changes needed: Node16 resolution supports explicit.jsimports
Root tsconfig extends@tsconfig/node20, which sets"moduleResolution": "node16", and in TS’s Node16/NodeNext modes explicit.jsspecifiers are fully supported (npmjs.com, typescriptlang.org).benchmark/react/package.json (1)
10-16: 003-hello-list target wiring verified
Entry “003-hello-list” is defined in benchmark/react/lynx.config.js and both dist/003-hello-list.lynx.bundle and .ptrace are produced as expected.benchmark/react/rspeedy-env.d.ts (2)
25-30: Update list parameter to FiberElement in __UpdateListCallbacks
In benchmark/react/rspeedy-env.d.ts, change thelistparameter from an array to a singleFiberElementto match the runtime signature:-declare function __UpdateListCallbacks( - list: FiberElement[], +declare function __UpdateListCallbacks( + list: FiberElement, componentAtIndex: ComponentAtIndexCallback, enqueueComponent: EnqueueComponentCallback, componentAtIndexes: ComponentAtIndexesCallback, ): void;
32-39: Confirm ComponentAtIndexCallback return typeUpstream Element.ts may define this callback as returning void. Verify the engine ABI and, if it doesn’t actually return a number, update the signature in benchmark/react/rspeedy-env.d.ts (lines 32–39) to
=> number | undefined.benchmark/react/lynx.config.js (1)
35-39: New 003-hello-list entry looks goodOrder places patches before the case; aligns with intended instrumentation.
benchmark/react/src/patchUpdateListCallbacks.ts (1)
39-77: Guard hook registrations on target existence
Wrap thehookcalls in existence checks to prevent calling an undefinedoldwhen the engine hasn’t injected these methods:if (typeof (globalThis as any).__UpdateListCallbacks === 'function') { hook( globalThis as any, '__UpdateListCallbacks' as any, (old, list, componentAtIndex, enqueueComponent, componentAtIndexes) => { old!( list, withCodspeed(componentAtIndex), enqueueComponent, componentAtIndexes, ); }, ); } if (typeof (globalThis as any).__CreateList === 'function') { hook( globalThis as any, '__CreateList' as any, (old, id, componentAtIndex, enqueueComponent, info, componentAtIndexes) => { return old!( id, withCodspeed(componentAtIndex), enqueueComponent, info, componentAtIndexes, ); }, ); }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
benchmark/react/src/hook.ts (1)
18-18: Normalize path separators for cross‑platform robustnessIf REPO_FILEPATH ever contains backslashes (Windows), split('/') will misparse. Normalize first.
Apply this diff:
-export const PREFIX = __REPO_FILEPATH__.split('/').slice(0, -2).join('/'); +export const PREFIX = __REPO_FILEPATH__.replace(/\\/g, '/').split('/').slice(0, -2).join('/');
📜 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 (10)
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/lynx.config.js(1 hunks)benchmark/react/package.json(1 hunks)benchmark/react/rspeedy-env.d.ts(1 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)
🚧 Files skipped from review as they are similar to previous changes (9)
- benchmark/react/cases/002-hello-reactLynx/index.tsx
- benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx
- benchmark/react/package.json
- benchmark/react/cases/003-hello-list/index.tsx
- benchmark/react/src/patchProfile.ts
- packages/lynx/benchx_cli/scripts/build.mjs
- benchmark/react/lynx.config.js
- benchmark/react/rspeedy-env.d.ts
- benchmark/react/src/patchUpdateListCallbacks.ts
🔇 Additional comments (2)
benchmark/react/src/hook.ts (2)
18-18: Shared PREFIX export looks goodCentralizing PREFIX here reduces drift across modules and simplifies future changes.
18-18: Declaration and imports verified
__REPO_FILEPATH__is declared inrspeedy-env.d.ts,PREFIXis only exported fromhook.ts, and all consumers import it from./hook.js. No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/lynx/benchx_cli/scripts/prepare.mjs (1)
5-13: Portability nit: avoid relying on /usr/bin/trueOn some distros, /usr/bin/true may not exist (only /bin/true). Consider emitting a tiny POSIX stub and ensuring it’s executable. Example:
import { chmodSync, writeFileSync } from 'node:fs'; // ... if (process.platform !== 'win32') { writeFileSync('./dist/bin/benchx_cli', '#!/bin/sh\nexit 0\n'); chmodSync('./dist/bin/benchx_cli', 0o755); }benchmark/react/cases/003-hello-list/index.tsx (2)
14-35: Type the ref as nullable and drop the non-null assertionPrevents potential runtime surprises and cleans up the effect.
- const listRef = useRef<NodesRef>(null); + const listRef = useRef<NodesRef | null>(null); useEffect(() => { - listRef.current!.invoke({ + const ref = listRef.current; + if (!ref) return; + ref.invoke({ method: 'scrollToPosition', params: { position: 1, smooth: false, }, - success: function() { - listRef.current!.invoke({ + success: function() { + ref.invoke({ method: 'scrollToPosition', params: { position: 2, smooth: false, }, success: function() { setStopBenchmark(true); }, }).exec(); }, }).exec(); }, []);
51-58: Add React keys to list itemsPrevents reconciliation warnings and keeps identity stable across updates.
- <list-item + <list-item + key={index} item-key={`${index}`}
📜 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 (10)
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/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)
🚧 Files skipped from review as they are similar to previous changes (7)
- benchmark/react/src/hook.ts
- benchmark/react/cases/002-hello-reactLynx/RecursiveText.tsx
- benchmark/react/src/patchProfile.ts
- packages/lynx/benchx_cli/scripts/build.mjs
- benchmark/react/cases/002-hello-reactLynx/index.tsx
- benchmark/react/lynx.config.js
- benchmark/react/src/patchUpdateListCallbacks.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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
🔇 Additional comments (5)
packages/lynx/benchx_cli/scripts/prepare.mjs (1)
9-13: Idempotence guard looks goodSkipping when the target already exists prevents needless overwrites and helps cacheable CI steps.
benchmark/react/cases/003-hello-list/index.tsx (1)
15-35: Consider useLayoutEffect for deterministic pre-paint scrolling (bench stability)If you need scrolls to occur before first paint for consistent traces, switch to useLayoutEffect.
Do you want me to patch this now or keep useEffect for post-paint behavior?
benchmark/react/package.json (3)
10-10: Scripts align with DOM id contractThe wait-for-id matches the component’s id toggle; perfetto entries mirror bench entries. LGTM.
Also applies to: 16-17
10-10:003-hello-listtarget is configured in lynx.config.js
lynx.config.js defines a003-hello-listentry pointing to./cases/003-hello-list/index.tsx, sodist/003-hello-list.lynx.bundlewill be generated.
31-31: No runtime imports detected; keep as devDependency
Search across TS, TSX, JS, and JSX found no non-import typeusage of@lynx-js/type-element-apiin benchmark/react.
Summary by CodeRabbit
New Features
Chores
Checklist