Skip to content

Defer idle requests made during a period unless the requesting callback used its slice - #130

Closed
mattcosta7 wants to merge 1 commit into
mainfrom
mattcosta7/ric-defer-cheap-requeues
Closed

mattcosta7 wants to merge 1 commit into
mainfrom
mattcosta7/ric-defer-cheap-requeues

Conversation

@mattcosta7

Copy link
Copy Markdown
Member

Follow-up to #129.

Problem

Since #129, a callback registered during an idle period runs later in the same 10 ms period. A cheap callback that requeues itself, such as a poller with nothing to do, therefore runs back to back for the whole period, every frame.

github-ui hits this case. Its pull request diff loader polls an empty queue through requestIdleCallback for the life of the page in virtualized and single-file views (packages/pull-requests/utils/progressive-diffs-loader.ts). On Safari, which has no native requestIdleCallback and so runs this polyfill, that loop now holds about half a core.

Change

Requests made while a period is running now wait for the next period, as native does. One exception remains: a request made synchronously by a callback that used its whole slice can still run later in the same period. That keeps the throughput of while (deadline.timeRemaining() > 0) { ... } work loops, which is what #129's deviation was for.

unpainted is renamed to deferred, because it now holds both kinds of waiting request.

Evidence

Playwright WebKit 26.5 runs a lab harness that installs each polyfill build before page scripts. Each figure is the median over 5 interleaved runs, with the min-max range in brackets. CPU is the WebContent process tree, sampled with ps.

v4.5.0 v4.5.2 this PR (built lib/) Chrome native
Cheap sync poller, calls/s 125 24,995 [19,941-25,604] 60 38
Cheap sync poller, CPU 2.6% 55.2% [41.2-56.2] 9.3% [7.5-11.8] 0.3%
Poller that requeues after await, calls/s 125 60 60 21
timeRemaining() loop, 200 ms of work 401 ms 678 ms 681 ms 202 ms
Input to next paint during idle work, p90 93 ms 21 ms 20 ms 51 ms
Self-requeue loop, fixed 1 ms chunks x150 1,193 ms 241 ms 2,478 ms 7,537 ms
200 independent 0.2 ms callbacks 208 ms 317 ms 334 ms 50 ms

npm test passes: lint, tsc, and wtr with 131 tests. The requestidlecallback file passed 5 repeated runs. Against the old src, 5 of the new or changed tests fail, as intended.

Tradeoffs

  • Self-requeuing loops that do fixed-size chunks and ignore timeRemaining() now get one chunk per frame: 2.5 s instead of 0.24 s in the table. That is still 3x faster than native Chrome, where such loops get one chunk per idle period. An audit of github-ui found no such loop.
  • I also tried a minimum-duration threshold: keep continuations in the same period if the callback ran at least 1 ms. I rejected it. WebKit's performance.now() has 1 ms resolution (measured), so a 1 ms threshold cannot tell a poller from a short work chunk.
  • The two tests that prove a whole-slice continuation runs in the same period depend on how quickly the next MessageChannel task runs. They retry up to 5 times. A starved CI worker could still fail them.
  • Not tested: real Safari, iOS Safari, or production traffic.

…ck used its slice

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mattcosta7
mattcosta7 marked this pull request as ready for review November 4, 2026 18:07
@mattcosta7
mattcosta7 requested a review from a team as a code owner November 4, 2026 18:07
@mattcosta7 mattcosta7 self-assigned this Nov 4, 2026
@mattcosta7
mattcosta7 requested review from jibrang and an auto review from Copilot November 4, 2026 18:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The test helper can accept an inline continuation before the outer callback’s microtasks, leaving microtask ordering unverified for successful same-period attempts.

1 open issue
What changed in this PR

Refines the idle scheduling introduced in #129 to prevent cheap self-rescheduling callbacks from monopolizing an idle period, while preserving throughput for callbacks that exhaust their slice.

Changes:

  • Defer requests made during a running period, except synchronous continuations from callbacks that consume their full slice.
  • Rename unpainted to deferred and track requests created by the running callback.
  • Extend tests for deferral, whole-slice continuations, polling frequency, and throwing callbacks.
File Description
test/​requestidlecallback.js Updates scheduling expectations and adds continuation, polling, and error-path coverage.
src/​requestidlecallback.ts Implements conditional deferral based on whether the requesting callback exhausted its slice.

Review effort: Max (auto)

Comment on lines +409 to +418
let continued = false
requestIdleCallback(deadline => {
exhaust(deadline)
requestIdleCallback(() => {
continued = true
})
})
runFrame()
await sleep(50)
if (continued) return true
@mattcosta7

Copy link
Copy Markdown
Member Author

Closing in favor of reverting #129 (#131). On real github.com pages, idle work is too small for the scheduler to change input latency, so there's nothing left for this fix to protect. If #129's approach comes back, this deferral rule should come with it.

@mattcosta7 mattcosta7 closed this Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants