[OPIK-8005] [FE] perf: cut per-cell store subscriptions 5x, skip no-op dataset hydration - #7934
[OPIK-8005] [FE] perf: cut per-cell store subscriptions 5x, skip no-op dataset hydration#7934Nimrod007 wants to merge 2 commits into
Conversation
…p dataset hydration Two contained changes for the Playground large-dataset slowdown. Both are behaviour-preserving; neither touches layout. This is partial relief, not a fix for OPIK_8005 — the unvirtualized output grid remains the structural problem. PlaygroundOutputCell subscribed to the store five times per cell (value, isLoading, stale, traceId, selectedRuleIds). All five of those hooks delegate to the same useOutputByPromptDatasetItemId selector, and the field access happens outside the selector, so zustand was comparing the same output object five times over. Every streaming token therefore ran that selector (dataset items x prompts x 5) times — roughly 4,600 evaluations per token on a 458-row dataset with two prompt variants. Read the output object once and pick the fields off it instead. The defaults applied here are the ones those hooks applied, so re-render behaviour is unchanged; there are simply 5x fewer subscriptions. The five hooks stay exported, as PlaygroundPromptOutput still uses three of them. useIncrementalDatasetHydration looped every row and rebuilt the whole array per iteration, but hydrateDatasetItemData only fetches when the item carries truncated media. On a text/JSON dataset every iteration was a no-op, so a 1000-row dataset did ~1M array copies to arrive back at the data it started with. Select the media-bearing indexes up front and loop only over those. Sized against a real production workspace: datasets there run to 1078 items and carry zero truncated media, so the hydration loop was pure waste. Refs: OPIK_8005, CUST_6878, AI_546 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
⏱️ pre-commit per-hook timing
⏭️ 38 skipped (no matching files changed)
|
|
No test needed here. Read both files and this is behaviour-preserving. In PlaygroundOutputCell the five hooks you collapsed are each literally Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 20 Aug 13:18 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| if (indexesToHydrate.length === 0) { | ||
| setIsHydrating(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stale media fetch overwrites new dataset
The no-media early-return path resets the shared cancelledRef.current to false without installing a replacement cleanup, so an earlier hydrateItems continuation can pass its post-await cancellation check and call setHydratedItems on the new array, overwriting it with stale data or updating state after unmount — should we use a generation-specific cancellation token, or install cleanup on every effect path?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-frontend/src/v2/pages/PlaygroundPage/useIncrementalDatasetHydration.ts`
around lines 42-45, fix the `useEffect` early-return path for datasets with no media.
Resetting the shared `cancelledRef` allows an older `hydrateItems` continuation to
update the replacement dataset or call setters after unmount; use a generation-specific
cancellation token/version, or install cleanup for every effect path, so stale async
work is always invalidated.
There was a problem hiding this comment.
Commit fb17f8e addressed this comment by replacing the shared cancellation ref with an effect-scoped cancellation token. Previous async hydrations are invalidated by cleanup before a new effect run, including no-media early returns and unmounts.
There was a problem hiding this comment.
Correct catch, and worth being precise about why it matters: this was a latent flaw that the no-media early return turned into a dangerous one.
cancelledRef is one ref shared across every effect run, and the effect body reset it to false on entry. React runs the previous run cleanup before the next body, so the sequence is: run 1 starts hydrating dataset A and installs cleanup -> dataset changes -> cleanup sets cancelled = true -> run 2 enters and resets it to false, un-cancelling run 1 still-pending loop -> run 1 continuation passes its post-await check and calls setHydratedItems on the new array.
Before this PR the only early return sat after setHydratedItems([]), so that stale continuation prev.map() ran over an empty array and produced nothing. Harmless, which is why it went unnoticed. The early return I added sits after setHydratedItems(datasetItems) with a populated array, so the same continuation writes dataset A data into dataset B array at a positional index — silent cross-dataset corruption, reachable by switching dataset or page size while a media-bearing dataset is still hydrating.
Fixed in fb17f8e by taking your second suggestion rather than a generation token: the flag is now scoped to each effect run (let cancelled = false) instead of being a shared ref. Each run observes only its own flag, so early-returning without installing cleanup cannot resurrect an older run loop, and no path needs its own cleanup to stay correct. That also fixes the pre-existing empty-dataset path and the setState-after-unmount case, and drops the useRef import.
Preferred it over a generation counter because it removes the shared mutable state entirely rather than versioning it, so the whole class of bug goes away instead of being guarded against.
One thing I did not do: there is no regression test. Reproducing it needs an in-flight media fetch interleaved with a dataset swap, and the hook has no existing test harness. The fix is reasoned against React cleanup ordering, not proven by test, so it is worth a human eye.
…(Baz review) Baz caught a real bug in the previous commit, and it is worth being precise that the new early-return path made a latent flaw dangerous rather than merely inelegant. cancelledRef is a single ref shared across every effect run, and the effect body reset it to false on entry. React runs the previous run's cleanup before the next body, so the sequence was: run 1 starts hydrating dataset A and installs cleanup -> dataset changes -> cleanup sets cancelled = true -> run 2 enters and resets it to false, un-cancelling run 1's still-pending loop -> run 1's continuation passes its post-await check and calls setHydratedItems on the new array. Before this PR the only early return sat after setHydratedItems([]), so that stale continuation's prev.map() ran over an empty array and produced nothing — harmless. The no-media early return added in the previous commit sits after setHydratedItems(datasetItems) with a populated array, so the same continuation writes dataset A's data into dataset B's array at a positional index. That is silent cross-dataset corruption, reachable by switching dataset or page size while a media-bearing dataset is still hydrating. Replace the shared ref with a `cancelled` flag scoped to each effect run. Each run observes only its own flag, so early-returning without installing cleanup cannot resurrect an older run's loop. This also fixes the pre-existing empty- dataset path and the setState-after-unmount case, and removes the useRef import. Verified: tsc, eslint --max-warnings=0, prettier, and the full vitest suite (150 files, 2229 tests) all pass. Refs: OPIK-8005, CUST_6878, AI_546 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nimrod007
changed the title
2 participants
Details
Two contained, behaviour-preserving performance changes for the Playground large-dataset slowdown, plus a cancellation-correctness fix from review. Neither perf change touches layout.
PlaygroundOutputCellsubscribed to the store five times per cell. All five hooks (value,isLoading,stale,traceId,selectedRuleIds) delegate to the sameuseOutputByPromptDatasetItemIdselector, and the field access happens outside the selector — so zustand was comparing the same output object five times over. Every streaming token ran that selector (dataset items x prompts x 5) times: roughly 4,600 evaluations per token on a 458-row dataset with two prompt variants. Now read the output object once and pick the fields off it.useIncrementalDatasetHydrationlooped every row and rebuilt the whole array per iteration, buthydrateDatasetItemDataonly fetches when an item carries truncated media. On a text/JSON dataset every iteration was a no-op, so a 1000-row dataset did ~1M array copies to arrive back at the data it started with. Now the media-bearing indexes are selected up front and only those are looped.Scope caution: this is partial relief, not a fix. The unvirtualized output grid — thousands of live
MarkdownPreviewsubtrees at 500+ rows — remains the structural problem and is not addressed here. Please do not close OPIK-8005 on this PR.Worth flagging for whoever picks up the structural work: the fix originally proposed on that ticket ("virtualize by passing
TableBody={DataTableVirtualBody}") will not work as written.DataTableVirtualBodyvirtualizes against a page-level scroll container fromusePageBodyScrollContainer(), andPlaygroundPageis not inside that provider. The context default is non-null, so nothing throws — the virtualizer just receives a null scroll element and renders zero rows.StickyScrollTablealso exists specifically becauseoverflow-x:autobreaksposition:sticky, the same conflict that blocks naive virtualization.Review follow-up
Baz flagged a stale-fetch bug on the no-media early return, and it was a correct catch worth stating precisely.
cancelledRefwas a single ref shared across every effect run, and the effect body reset it tofalseon entry. React runs the previous run's cleanup before the next body, so: run 1 starts hydrating dataset A and installs cleanup → dataset changes → cleanup setscancelled = true→ run 2 enters and resets it tofalse, un-cancelling run 1's still-pending loop → run 1's continuation passes its post-awaitcheck and callssetHydratedItemson the new array.Before this PR the only early return sat after
setHydratedItems([]), so the stale continuation'sprev.map()ran over an empty array and produced nothing — harmless. The no-media early return added here sits aftersetHydratedItems(datasetItems)with a populated array, so the same continuation writes dataset A's data into dataset B's array at a positional index. Silent cross-dataset corruption, reachable by switching dataset or page size while a media-bearing dataset is still hydrating.Fixed by scoping the flag to each effect run (
let cancelled = false) instead of sharing a ref. Each run observes only its own flag, so early-returning without installing cleanup can no longer resurrect an older run's loop. This also fixes the pre-existing empty-dataset path and the setState-after-unmount case.Change checklist
Issues
Partial relief only — OPIK-8005 should stay open for the virtualization work. Related but not resolved: CUST_6878 (customer report), AI_546 (investigation record).
AI-WATERMARK
AI-WATERMARK: yes
Testing
Commands run locally in
apps/opik-frontend(deps vianpm ci), re-run after the review fix:Equivalence reasoning for the subscription change (why it is safe, not merely green):
?? null,?? false,?? null, and no default forselectedRuleIds).updateOutputreplaces only the target leaf with a new object. Unrelated cell updates therefore still return an identical reference and still do not re-render this cell. Re-render behaviour is unchanged; the only difference is 5x fewer subscriptions.PlaygroundPromptOutput.tsxstill consumes three of them (single-instance, not per-row, so no benefit in changing it).Equivalence reasoning for the hydration change:
DatasetItem.datais a required field, so dropping the old defensive?? {}path changes nothing.dataobject, which is exactly whathydrateDatasetItemDatareturned for them.PlaygroundOutputTable, readshydratedItemsand ignoresisHydrating.Not run / not done, with reasons:
updateOutputduring streaming to coalesce per-token store writes) was deliberately left out. It is the only one that is not behaviour-neutral: it needs a flush-on-completion path, and if that is wrong a dropped final chunk silently corrupts a cell's output. It deserves its own PR with a measurement to justify it.Documentation
No documentation change. All changes are internal; no user-facing API, behaviour or UI surface change.