Hook the renderer into the scheduler through a classic-default adapter - #21553
Hook the renderer into the scheduler through a classic-default adapter#21553NullVoxPopuli-ai-agent wants to merge 2 commits into
Conversation
The scheduler interface gains its first consumer. The renderer now
always schedules revalidation through the active strategy via an
internal, callback-based seam (_scheduleRevalidate) -- revalidation is
hotter than any user phase, so the renderer does not allocate promises
per invalidation.
The ambient default is a new ClassicStrategy that reproduces today's
behavior exactly: revalidation is the same scheduleOnce('render', ...)
with the same dedupe (the flush callback is stable per renderer), and
the public phase functions map onto the runloop's queues (render ->
'render', layout -> 'afterRender', composite behind layout within the
same flush, next -> runloop next, idle -> requestIdleCallback with a
starvation timeout). No registration required; existing applications
observe no timing change -- the full suite passes unmodified.
registerStrategy remains the swap seam: a render-aware strategy that
implements _scheduleRevalidate takes over revalidation scheduling,
which is where the performance benefits arrive when such a strategy
becomes the default. A registered strategy without the seam leaves the
renderer on classic scheduling.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| _backburner.scheduleOnce('render', this, this.revalidate, renderer); | ||
| this.#renderer = renderer; | ||
|
|
||
| const strategy = _getStrategy(); |
There was a problem hiding this comment.
this should happen during construction time, i believe, because we don't want to re-check all this strategy stuff eveyr revalidation.
Additionally, I think you'll want to hook in to the environment, since that's where existing backburner integration is (and the scheduler is replacing backburner eventually
| * | ||
| * @internal | ||
| */ | ||
| export function _getStrategy(): Strategy { |
There was a problem hiding this comment.
this function is redundant and unneeded. don't be silly
| */ | ||
| export function render(): Promise<void> { | ||
| return getStrategy('render').render(); | ||
| return getStrategy().render(); |
There was a problem hiding this comment.
why call getStrategy so much when we can just directly access registeredStrategy?
There was a problem hiding this comment.
Agreed and removed: getStrategy is gone entirely. The phase functions and the renderer access the exported live binding _registeredStrategy directly (with a pre-boot assert in the phase functions).
| // the classic strategy is the ambient default: existing applications | ||
| // keep today's runloop scheduling with no registration required, and | ||
| // `registerStrategy` swaps in a render-aware implementation | ||
| return registeredStrategy ?? classicStrategy; |
There was a problem hiding this comment.
classicStrategy probably shouldn't be returned here (I'm questiening the entire purpose of getStrategy atm)
maybe we setStrategy in the environment (glimmer<->ember hookup) and pass the classicStrategy there
There was a problem hiding this comment.
Took exactly this direction in df40e9e: no ambient fallback anywhere — the glimmer<->ember hookup (environment.ts, next to setGlobalContext) does registerStrategy(classicStrategy) at initialization, so a booted app always has the classic strategy unless it swaps one in. registerStrategy permits one swap over the classic default and still asserts on conflicting registrations. Full suite 9425/0 and repo lint green with the rework.
Review feedback: no getStrategy indirection and no ambient fallback. The active strategy is an exported live binding the phase functions and the renderer access directly (with a pre-boot assert), and the classic default is established where the glimmer<->ember hookup happens -- registerStrategy(classicStrategy) at environment initialization. registerStrategy permits one application swap over the classic default and still asserts on conflicting registrations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * | ||
| * @internal | ||
| */ | ||
| _scheduleRevalidate?(flush: () => void): void; |
There was a problem hiding this comment.
why is this here? we can't add to the interface of the scheduler, even for compat
| @@ -1,4 +1,5 @@ | |||
| import { assert } from '@ember/debug'; | |||
| import classicStrategy from '@ember/scheduler/-private/classic'; | |||
There was a problem hiding this comment.
we should not import this here.
| registeredStrategy === null || registeredStrategy === strategy | ||
| _registeredStrategy === null || | ||
| _registeredStrategy === strategy || | ||
| _registeredStrategy === classicStrategy |
There was a problem hiding this comment.
this third comparison is unneeded, and would be handled by the comparison above
There was a problem hiding this comment.
to test this proper, you'd want a well known symbol to check on the internal strategies, because we do want those to be overwritten
| } | ||
| } | ||
|
|
||
| #renderer: BaseRenderer | null = null; |
There was a problem hiding this comment.
I do not believe we are done.
Because we haven't wired up all our internals to the scheduler, a user swapping out the scheduler would see no benefit.
#21520 made a lot of progress on that work, and you'll need to copy some of that here (though, only insofar as it applies to enabling the classic strategy.
This is also way too much code.
Read https://github.com/runspired/rfcs/blob/modernized-scheduler/text/0957-modernized-scheduler.md again
|
temporray misunderstanding |
Targets #21552's branch: the interface gains its first consumer, wired so that today's behavior is the default and the perf benefits arrive by swapping the strategy later.
What this adds
Strategy: optional_scheduleRevalidate(flush)(underscore,@internal). The renderer always schedules revalidation through the active strategy via this callback-based hook — deliberately not the public promise API, since revalidation is hotter than any user phase and promise-per-invalidation overhead was measured as a real cost class during the SPIKE: every rendering-performance lever combined (RFC 957 end state + VM optimizations) #21520 spike.ClassicStrategyas the ambient default (private module, no registration required): revalidation is byte-for-byte today'sscheduleOnce('render', …)with the same dedupe semantics (stable flush callback per renderer), and the public phase functions become meaningful under classic timing —render()→ therenderqueue,layout()→afterRender,composite()re-scheduled behind layout's entries within the same flush,next()→ runloopnext,idle()→requestIdleCallbackwith a starvation timeout (fully-idle pages never grant bare rIC — the Implement RFC #957: Render Aware Scheduler Interface #21493 lesson).getStrategyfalls back to classic instead of asserting;registerStrategykeeps its double-registration assert and remains the swap seam. A registered strategy without the internal seam leaves the renderer on classic scheduling.Verification
render → layout → compositeorder under runloop semantics), plus a seam-delegation test.The path this sets up
When a render-aware strategy (the #21520 spike's clock: microtask-window chain classification, unclamped-macrotask/rAF race, one-render-per-tick) implements
_scheduleRevalidateand becomes the default, the renderer picks up coalesced, frame-aligned revalidation with no further renderer changes — that swap is where the spike's measured wins (6–24x async, 2.5x dbmon) transfer.🤖 Generated with Claude Code