Yield while parsing data so that the UI isn't blocked while loading larger shapes #3540
Conversation
…hapes Add async parsing methods that yield to the main thread periodically, preventing the UI from becoming unresponsive when syncing shapes with many rows. - Add yieldToMain() utility that uses scheduler.yield() when available, falling back to setTimeout(0) - Add parseAsync() and parseSnapshotDataAsync() methods to MessageParser that yield every 1000 messages by default - Update ShapeStream to always use async parsing for long-poll responses and snapshots - Export yieldToMain and DEFAULT_YIELD_EVERY for users who need custom yielding in their own processing code
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #3540 +/- ##
===========================================
- Coverage 75.24% 61.64% -13.61%
===========================================
Files 51 40 -11
Lines 2743 1559 -1184
Branches 404 106 -298
===========================================
- Hits 2064 961 -1103
+ Misses 677 597 -80
+ Partials 2 1 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
- Remove flaky timing assertion in yield test that assumed specific setTimeout ordering - Run prettier on modified files
- Add yield test for parseAsync to improve coverage (was only testing parseSnapshotDataAsync yields) - Fix globalThis guard order in yieldToMain - move typeof check before assignment to avoid reference errors in exotic environments
Remove the yieldEvery parameter from parseAsync and parseSnapshotDataAsync methods - yield interval is now fixed at 1000 messages internally. This simplifies the API; configurability can be added later if users need it. - Remove yieldEvery parameter from async parsing methods - Remove DEFAULT_YIELD_EVERY from public exports - Update tests to work with fixed yield interval
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
The code looks good but i'm worried about the arbitrary yield every 1000 messages. Depending on the messages we may be yielding too often or not often enough. I think a time-based yielding strategy would be more appropriate. At 60 FPS we basically have 16.6ms per frame so we should yield every 10ms (maximum). Something like this:
const BUDGET = 8; // ms
let start = performance.now();
for (let i = 0; i < msgs.length; i++) {
// do work
if (performance.now() - start > BUDGET) {
await yieldToMain()
start = performance.now()
}
}| this.transformParsedMessage(parsed[i], schema) | ||
|
|
||
| // Yield periodically to prevent blocking | ||
| if ((i + 1) % DEFAULT_YIELD_EVERY === 0) { |
There was a problem hiding this comment.
nit: i'd prefer if parseAsync and parseSnapshotDataAsync would take the yield every configuration as an argument instead of relying on a global. We can still have a global configuration but i would pass that global as an argument when calling these functions.
|
It's just a setTimeout which returns essentially immediately unless something else needs a bunch of CPU — so essentially free to do whenever. I'm not sure the interval really matters? |
I get what you mean about Even if the yield itself is “cheap,” it isn’t free. Every time we yield we’re:
That context-switching overhead adds up if we do it far more often than necessary. Yielding every 1000 messages might mean we yield thousands of times per second on fast hardware or large batches, which is unnecessary churn. On the other hand, if each message is expensive, yielding every 1000 iterations might mean we block the main thread for much longer than intended—possibly tens of milliseconds—which risks jank. The browser’s main thread time budget is fundamentally time-based, not iteration-based, so a time-based yielding strategy matches the actual constraint:
The goal is not to yield “whenever,” but to yield “often enough to keep the app responsive, while not yielding so often that we waste time context switching.” |
Add async parsing methods that yield to the main thread periodically, preventing the UI from becoming unresponsive when syncing shapes with many rows.