schedule event flush - #1052
Conversation
🦋 Changeset detectedLatest commit: 4606ebf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 5m 30s —— View job Review
The design is clean and well-motivated. Bug
Design concernScheduler and budget types don't belong in
Nits
ObservationThere is no |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const flush = () => { | ||
| const start = performance.now() | ||
| const applied = pending.size | ||
| for (const [uuid, change] of pending) { | ||
| applyChange(uuid, change) | ||
| } | ||
| pending.clear() | ||
|
|
||
| if (applied > 0) invalidate() | ||
| stats.recordFlush({ start, end: performance.now(), applied, backlog: 0 }) | ||
| } | ||
|
|
||
| const scheduleFlush = () => { | ||
| if (flushScheduled) return | ||
| flushScheduled = true | ||
| const result = drainWithBudget(pending, applyChange, { | ||
| now: () => performance.now(), | ||
| budgetMs: FLUSH_BUDGET_MS, | ||
| maxSpawns: FLUSH_MAX_SPAWNS, | ||
| }) | ||
|
|
||
| rafId = requestAnimationFrame(() => { | ||
| rafId = 0 | ||
| flushScheduled = false | ||
| flush() | ||
| if (result.applied > 0) invalidate() | ||
| stats.recordFlush({ | ||
| start, | ||
| end: performance.now(), | ||
| applied: result.applied, | ||
| backlog: result.remaining, | ||
| }) | ||
| if (result.remaining > 0) scheduler.request() | ||
| } |
There was a problem hiding this comment.
Bug: throw from drainWithBudget leaves remaining entries stuck.
drainWithBudget deliberately re-throws when apply throws (the entry is pre-deleted so it won't wedge on the next drain, but the throw propagates — see the test at budgetedFlush.spec.ts:76). When that throw bubbles up through flush, scheduler.request() on this line is never reached. The rAF/timer handle was already cleared in run() before deps.flush() was called, so nothing re-schedules the drain. Any entries that were still in pending sit there until a new stream event arrives and calls scheduler.request() from consumeChanges. In a static scene (stream idle), those entries never drain.
Suggested fix — wrap the drain in a try/finally so the reschedule is guaranteed:
| const flush = () => { | |
| const start = performance.now() | |
| const applied = pending.size | |
| for (const [uuid, change] of pending) { | |
| applyChange(uuid, change) | |
| } | |
| pending.clear() | |
| if (applied > 0) invalidate() | |
| stats.recordFlush({ start, end: performance.now(), applied, backlog: 0 }) | |
| } | |
| const scheduleFlush = () => { | |
| if (flushScheduled) return | |
| flushScheduled = true | |
| const result = drainWithBudget(pending, applyChange, { | |
| now: () => performance.now(), | |
| budgetMs: FLUSH_BUDGET_MS, | |
| maxSpawns: FLUSH_MAX_SPAWNS, | |
| }) | |
| rafId = requestAnimationFrame(() => { | |
| rafId = 0 | |
| flushScheduled = false | |
| flush() | |
| if (result.applied > 0) invalidate() | |
| stats.recordFlush({ | |
| start, | |
| end: performance.now(), | |
| applied: result.applied, | |
| backlog: result.remaining, | |
| }) | |
| if (result.remaining > 0) scheduler.request() | |
| } | |
| const flush = () => { | |
| const start = performance.now() | |
| let result: DrainResult | undefined | |
| try { | |
| result = drainWithBudget(pending, applyChange, { | |
| now: () => performance.now(), | |
| budgetMs: FLUSH_BUDGET_MS, | |
| maxSpawns: FLUSH_MAX_SPAWNS, | |
| }) | |
| if (result.applied > 0) invalidate() | |
| stats.recordFlush({ | |
| start, | |
| end: performance.now(), | |
| applied: result.applied, | |
| backlog: result.remaining, | |
| }) | |
| } finally { | |
| if (pending.size > 0) scheduler.request() | |
| } | |
| } |
(You'd need to import DrainResult from pendingTransformChanges.) This also keeps stats.recordFlush inside the try so a throw there doesn't block the reschedule either.
| if (existing) destroyEntity(uuid) | ||
| spawnEntity(change.transform) | ||
| return | ||
| return { spawned: true } |
There was a problem hiding this comment.
Nit: destroy + spawn counts as one lifecycle op toward the cap, but two are queued.
When an existing entity has a mismatched referenceFrame, destroyEntity is called (queuing Threlte teardown) and then spawnEntity (queuing Threlte mount). The microtask checkpoint after the callback runs both. { spawned: true } counts this pair as one toward FLUSH_MAX_SPAWNS, so the cap is slightly loose for this REMOVED-then-ADDED republish pattern.
This is an edge case and probably fine in practice, but if you want strict bounding here you could return a separate count or a boolean-pair instead of the single spawned flag.
| export interface FlushScheduler { | ||
| /** Idempotent: a flush already scheduled is not scheduled twice. */ | ||
| request(): void | ||
| cancel(): void | ||
| } | ||
|
|
||
| /** The environment a scheduler runs in, injectable so tests can drive it. */ | ||
| export interface FlushSchedulerDeps { | ||
| flush: () => void | ||
| isVisible: () => boolean | ||
| requestFrame: (callback: () => void) => number | ||
| cancelFrame: (handle: number) => void | ||
| setTimer: (callback: () => void, ms: number) => number | ||
| clearTimer: (handle: number) => void | ||
| hiddenIntervalMs: number | ||
| } |
There was a problem hiding this comment.
Design concern: scheduler types don't belong in this file.
FlushScheduler and FlushSchedulerDeps are the contract for flushScheduler.ts — nothing outside those two files needs to import them by name. They'd be more findable and better scoped sitting at the top of flushScheduler.ts.
Similarly, FlushBudget, DrainResult, and ApplyOutcome are the contract for budgetedFlush.ts. WorldState.svelte imports ApplyOutcome from here but only because that's where it landed; moving it to budgetedFlush.ts and adjusting the import is a one-liner.
Leaving them here turns pendingTransformChanges.ts into a secondary types hub. The file already has a clear job (the pending-map data model and topLevelField). The flush types aren't part of that job.
|
|
||
| /** What applying one pending change did; a destroy counts as a spawn for budgeting. */ | ||
| export interface ApplyOutcome { | ||
| spawned: boolean |
There was a problem hiding this comment.
Nit: spawned is true for destroys, which the interface comment has to explain away.
The field is used as a proxy for "a Threlte lifecycle operation was queued", but the name reads as spawn-only. A name like lifecycleOp would be self-explanatory at every callsite without needing the doc comment to bridge the gap.
| spawned: boolean | |
| export interface ApplyOutcome { | |
| lifecycleOp: boolean | |
| } |
(Touch drainWithBudget.ts and WorldState.svelte to rename outcome.spawned → outcome.lifecycleOp and spawns → lifecycleOps, then update the DrainResult.spawns field too.)
| const run = (): void => { | ||
| frameHandle = undefined | ||
| timerHandle = undefined | ||
| deps.flush() | ||
| } |
There was a problem hiding this comment.
Observation: run clears both handles before calling flush, so a throw in flush leaves no rescheduled work.
frameHandle and timerHandle are both set to undefined here before deps.flush() is called. That's correct for the idempotency check — you don't want a second request() inside flush to be a no-op because the old handle is still set. But it also means that if deps.flush() throws, the scheduler is in a clean-handles state with no future work scheduled. The fix lives on the flush side (see the comment on WorldState.svelte:268), but it's worth knowing the scheduler itself provides no safety net here.
|
|
||
| import { drainWithBudget } from '../budgetedFlush' | ||
|
|
||
| // The drain never reads the transform, so a stub carrying only the key is enough. |
There was a problem hiding this comment.
Edge-case: this comment is on the line of the "no comments in tests" rule from testing.md. It survives the rule's narrow exception ("rare domain fact a reader could not infer"), but it is borderline — the stub's type signature and the name makeChange do most of the work. Could drop the comment; the function name says enough.
1 participant
Applies coalesced world state changes under a per-frame budget, carrying whatever does not fit to the next frame, so a burst of changed entities costs a bounded slice of each frame instead of one long one. Keeps draining while the tab is hidden. Stacks on the coalescing PR (
ws3-coalesce).World state
drainWithBudgetinbudgetedFlush.tsapplies pending changes in insertion order and stops after the entry that crossesFLUSH_BUDGET_MS(6 ms) or afterFLUSH_MAX_SPAWNS(16) spawns and destroys, whichever comes first. It deletes each entry before applying it, so an apply that throws drops its entry instead of wedging the queue.createFlushSchedulerinflushScheduler.tsruns the flush onrequestAnimationFramewhile the document is visible and on aHIDDEN_FLUSH_INTERVAL_MS(250 ms) timer while it is hidden, with an idempotentrequestand acancel.WorldStateswaps its ownrequestAnimationFramebookkeeping for the scheduler, drains throughdrainWithBudget, reports the remainder as the stats folder'sBacklog, and requests another frame while anything is left.applyChangereports whether it spawned or destroyed, which is what the spawn cap counts.Why?
Why a spawn cap next to a millisecond budget?
The expensive part of a spawn or destroy is the Threlte mount or teardown that Svelte runs in the microtask checkpoint after the callback returns. A wall-clock budget measured inside the callback cannot see it, so the cap bounds it by count.
Why keep flushing while hidden?
requestAnimationFramepauses in a hidden tab. The pending map is bounded by entity count, so this is about latency on return, not memory: a 250 ms timer keeps the scene current enough that switching back does not replay a backlog.Testing
Ran
pnpm check,pnpm test, andpnpm lintthrough verify-changed. AddedbudgetedFlush.spec.ts, with an injected clock showing the budget stopping mid-map and resuming next frame, andflushScheduler.spec.ts, driving both the frame and the hidden-timer paths through injected deps. I did not runpnpm e2e:robot.