ci(benchmark/react): background setAttribute BatchedValues#1655
ci(benchmark/react): background setAttribute BatchedValues#1655hzy merged 1 commit intolynx-family:mainfrom
BatchedValues#1655Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughExports BackgroundSnapshotInstance from runtime internals, instruments BackgroundSnapshotInstance.prototype.setAttribute (and main-thread SnapshotInstance) to start/stop Codspeed benchmarks on a batched 'stop-benchmark-true' sentinel, and simplifies main-thread detection in the benchmark hook. 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
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds background thread support for benchmarking setAttribute operations on BatchedValues in the React runtime. The changes enable performance testing of attribute updates that occur in background workers, complementing existing main thread benchmarking.
- Exports
BackgroundSnapshotInstancefrom the React runtime internal module - Adds background thread benchmarking hooks for
setAttributeoperations - Cleans up TypeScript comment formatting in hook utility
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/react/runtime/src/internal.ts |
Adds import and export of BackgroundSnapshotInstance to expose it for benchmarking |
packages/react/runtime/lazy/internal.js |
Exports BackgroundSnapshotInstance in the lazy loading module |
benchmark/react/src/hook.ts |
Removes unnecessary TypeScript error suppression comment |
benchmark/react/cases/004-various-update/index.tsx |
Adds background thread benchmarking hooks and updates main thread condition check |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmark/react/cases/004-various-update/index.tsx (1)
78-85: Always stop the benchmark viafinally.If
old.callthrows,Codspeed.stopBenchmark()andsetExecutedBenchmark()won’t run, leaving the harness in a bad state.Apply:
- Codspeed.startBenchmark(); - const ret = old!.call(this, key, value); - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, - ); - - return ret; + Codspeed.startBenchmark(); + try { + return old!.call(this, key, value); + } finally { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, + ); + }
🧹 Nitpick comments (3)
benchmark/react/src/hook.ts (1)
20-20: Avoid TS error by accessing the injected global viaglobalThis.Directly referencing
__CreatePagewithout an ambient declaration can fail type-checking after removing@ts-expect-error. UseglobalThis(or add a.d.tsambient) to keep this safe.Apply:
-export const isMainThread = typeof __CreatePage === 'function'; +export const isMainThread = typeof (globalThis as any).__CreatePage === 'function';Optionally, add an ambient in a globals.d.ts:
declare const __CreatePage: unknown;packages/react/runtime/src/internal.ts (1)
21-21: Public surface area expanded — add a changeset entry.Exporting
BackgroundSnapshotInstancealters the package API. Please include a changeset (likely minor) and note in internal docs.benchmark/react/cases/004-various-update/index.tsx (1)
105-106: Optional: de-stringify the sentinel.Using a boolean sentinel (or a dedicated prop) is less brittle than matching
'stop-benchmark-true'.
📜 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(2 hunks)benchmark/react/src/hook.ts(1 hunks)packages/react/runtime/lazy/internal.js(1 hunks)packages/react/runtime/src/internal.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/src/internal.ts
📚 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:
packages/react/runtime/src/internal.tsbenchmark/react/cases/004-various-update/index.tsx
📚 Learning: 2025-08-13T11:46:43.737Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1523
File: vitest.config.ts:5-6
Timestamp: 2025-08-13T11:46:43.737Z
Learning: In the lynx-stack codebase, default imports are consistently used for Node.js built-in modules (e.g., `import os from 'node:os'`, `import fs from 'node:fs'`). The TypeScript configuration supports esModuleInterop and allowSyntheticDefaultImports, making default imports the preferred pattern over namespace imports for Node.js built-ins.
Applied to files:
packages/react/runtime/src/internal.ts
🧬 Code graph analysis (1)
benchmark/react/cases/004-various-update/index.tsx (2)
benchmark/react/src/hook.ts (1)
hook(5-16)packages/react/runtime/src/internal.ts (1)
BackgroundSnapshotInstance(21-21)
🔇 Additional comments (5)
packages/react/runtime/lazy/internal.js (1)
8-8: VerifyBackgroundSnapshotInstanceexists on all targets.This re-export assumes
target[sExportsReactInternal].BackgroundSnapshotInstanceis present across all runtimes. Please confirm the symbol is wired in every build variant; otherwise downstream imports may getundefined.packages/react/runtime/src/internal.ts (1)
10-10: Ensure the source module actually exportsBackgroundSnapshotInstance.Double-check
./backgroundSnapshot.jsnamed-exportsBackgroundSnapshotInstanceand that tree-shaking/cycles won’t elide it.benchmark/react/cases/004-various-update/index.tsx (3)
6-9: Import looks correct; confirm type availability.Good to import alongside
SnapshotInstance. Ensure@lynx-js/react/internaltypes includeBackgroundSnapshotInstancein all distributions.
14-14: Guard on compile-time macros may throw if not defined in non-Lynx envs.Using
__MAIN_THREAD__directly relies on compile-time replacement. If this file runs outside the Lynx bundling context (e.g., plain Node/Vitest), it canReferenceError. Confirm benchmarks always build with these macros.
94-94: Correctthistyping for background hook.Typing
this: BackgroundSnapshotInstancematches the hooked prototype. Looks good.
Web Explorer#4847 Bundle Size — 367.43KiB (0%).e9caf02(current) vs 14d9475 main#4845(baseline) Bundle metrics
|
| Current #4847 |
Baseline #4845 |
|
|---|---|---|
 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.32% |
3.32% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4847 |
Baseline #4845 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch hzy:p/hzy/bench_6 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4856 Bundle Size — 237.5KiB (0%).e9caf02(current) vs 14d9475 main#4854(baseline) Bundle metrics
|
| Current #4856 |
Baseline #4854 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
163(+1.88%) |
160 |
 Duplicate Modules |
68(+4.62%) |
65 |
| Duplicate Code |
46.97%(+2.49%) |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4856 |
Baseline #4854 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.74KiB |
91.74KiB |
Bundle analysis report Branch hzy:p/hzy/bench_6 Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1655 will not alter performanceComparing 🎉 Hooray!
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| 🆕 | 004-various-update-setAttribute__BatchedValues |
N/A | 317.2 µs | N/A |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
benchmark/react/cases/004-various-update/index.tsx (1)
87-110: Guard non-array values and usefinallyin the background path as well.Same reliability concern: ensure stop/label even if the call throws, and avoid
.lengthon non-arrays.- function(this: BackgroundSnapshotInstance, old, key, value) { - const values = value as unknown[]; - if ( - key === 'values' && values[values.length - 1] === 'stop-benchmark-true' - ) { + function(this: BackgroundSnapshotInstance, old, key, value) { + const v = value as unknown; + if ( + key === 'values' && + Array.isArray(v) && + v.length > 0 && + v[v.length - 1] === 'stop-benchmark-true' + ) { // we only care about the update that stops the benchmark - - Codspeed.startBenchmark(); - const ret = old!.call(this, key, value); - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, - ); - return ret; + Codspeed.startBenchmark(); + try { + return old!.call(this, key, value); + } finally { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, + ); + } } return old!.call(this, key, value); },
🧹 Nitpick comments (2)
benchmark/react/cases/004-various-update/index.tsx (2)
14-14: Unify main-thread detection or declare macro types.Prefer using the already-computed
isMainThreadfor consistency, or ensure__MAIN_THREAD__has an ambient type.-if (typeof Codspeed !== 'undefined' && __MAIN_THREAD__) { +if (typeof Codspeed !== 'undefined' && isMainThread) {
75-83: Always stop/label benchmarks even on throw.Wrap with
try/finallyto avoid leaking an active benchmark and missing labels if the underlying setter throws.- Codspeed.startBenchmark(); - const ret = old!.call(this, key, value); - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, - ); - - return ret; + Codspeed.startBenchmark(); + try { + return old!.call(this, key, value); + } finally { + 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 selected for processing (4)
benchmark/react/cases/004-various-update/index.tsx(6 hunks)benchmark/react/src/hook.ts(1 hunks)packages/react/runtime/lazy/internal.js(1 hunks)packages/react/runtime/src/internal.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/runtime/lazy/internal.js
- packages/react/runtime/src/internal.ts
🧰 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/cases/004-various-update/index.tsx
🧬 Code graph analysis (1)
benchmark/react/cases/004-various-update/index.tsx (2)
benchmark/react/src/hook.ts (1)
hook(5-16)packages/react/runtime/src/internal.ts (1)
BackgroundSnapshotInstance(21-21)
🔇 Additional comments (3)
benchmark/react/cases/004-various-update/index.tsx (3)
6-9: ConfirmBackgroundSnapshotInstanceavailability across build targets.This import assumes
@lynx-js/react/internalnow exportsBackgroundSnapshotInstance. Please verify the runtime/lazy and srcinternalexports are published in all bundles used by benchmarks.
56-64: Reindexing looks correct; verify enum alignment with runtime.Mapping 10→ListItemPlatformInfo, 11→ListItemPlatformInfoSpread, 12→Spread matches the described reindex. Please confirm the producer side emits these exact indices.
178-178: Prop reindexing in JSX matches the updated attribute map.Usage shifts (
main-thread:ref→ index 9;item-key→ index 10; spread comments updated) are consistent with the new mapping.Also applies to: 180-180, 190-190, 213-213, 215-215
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmark/react/cases/004-various-update/index.tsx (1)
78-86: Always stop/label benchmark via finally in main-thread path.If
setAttributethrows,stopBenchmarkand the label won’t run.Apply:
- Codspeed.startBenchmark(); - const ret = old!.call(this, key, value); - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, - ); - - return ret; + Codspeed.startBenchmark(); + try { + return old!.call(this, key, value); + } finally { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__${name}`, + ); + }
♻️ Duplicate comments (3)
benchmark/react/cases/004-various-update/index.tsx (2)
95-98: Guard against non-arrayvaluebefore accessing.length.Prevents runtime errors when
valueisn’t an array.Apply:
- const values = value as unknown[]; - if ( - key === 'values' && values[values.length - 1] === 'stop-benchmark-true' - ) { + if ( + key === 'values' && + Array.isArray(value) && + value.length > 0 && + value[value.length - 1] === 'stop-benchmark-true' + ) {
101-107: Use try/finally in background path as well.Ensure stop/label even if
old!.callthrows.Apply:
- Codspeed.startBenchmark(); - const ret = old!.call(this, key, value); - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, - ); - return ret; + Codspeed.startBenchmark(); + try { + return old!.call(this, key, value); + } finally { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, + ); + }benchmark/react/src/hook.ts (1)
20-20: Fix TS global typing for__CreatePage(or access via globalThis).Removing
@ts-expect-errorreintroduces “Cannot find name '__CreatePage'” without an ambient.Apply one:
-export const isMainThread = typeof __CreatePage === 'function'; +export const isMainThread = typeof (globalThis as any).__CreatePage === 'function';Or add an ambient (recommended):
// benchmark/react/globals.d.ts declare const __CreatePage: unknown; declare const __MAIN_THREAD__: boolean; declare const __BACKGROUND__: boolean; declare const __REPO_FILEPATH__: string; declare const __webpack_chunkname__: string; declare const Codspeed: | { startBenchmark(): void; stopBenchmark(): void; setExecutedBenchmark(name: string): void } | undefined;Ensure tsconfig includes this d.ts.
🧹 Nitpick comments (1)
benchmark/react/cases/004-various-update/index.tsx (1)
105-105: Disambiguate background vs main labels.Optional: prefix background labels to avoid future name collisions in reports.
Apply:
- `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, + `${__REPO_FILEPATH__}::BTS::${__webpack_chunkname__}-setAttribute__BatchedValues`,
📜 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(2 hunks)benchmark/react/src/hook.ts(1 hunks)packages/react/runtime/lazy/internal.js(1 hunks)packages/react/runtime/src/internal.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/internal.ts
🧰 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/cases/004-various-update/index.tsx
🧬 Code graph analysis (1)
benchmark/react/cases/004-various-update/index.tsx (2)
benchmark/react/src/hook.ts (1)
hook(5-16)packages/react/runtime/src/internal.ts (1)
BackgroundSnapshotInstance(21-21)
🔇 Additional comments (3)
packages/react/runtime/lazy/internal.js (1)
7-9: BackgroundSnapshotInstance export verified
BackgroundSnapshotInstance is exported from packages/react/runtime/src/internal.ts, implemented in backgroundSnapshot.ts, and present in packages/react/runtime/lazy/internal.js.benchmark/react/cases/004-various-update/index.tsx (2)
6-9: LGTM: internal import added for background hooks.Importing
BackgroundSnapshotInstancehere aligns with the new runtime export.
90-113: Drop the numeric‐enum concern: ‘values’ is explicitly used as a string key insetAttributeand the implementation accepts string keys.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Checklist