-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Hook the renderer into the scheduler through a classic-default adapter #21553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import type { SimpleDocument, SimpleElement } from '@simple-dom/interface'; | |
| import { hasDOM } from '../../browser-environment'; | ||
| import { EmberEnvironmentDelegate } from './environment'; | ||
| import ResolverImpl from './resolver'; | ||
| import { _getStrategy } from '@ember/scheduler'; | ||
| import { EvaluationContextImpl } from '@glimmer/opcode-compiler/lib/program-context'; | ||
|
|
||
| export type IBuilder = (env: Environment, cursor: Cursor) => TreeBuilder; | ||
|
|
@@ -368,8 +369,28 @@ export class RendererState { | |
| } | ||
| } | ||
|
|
||
| #renderer: BaseRenderer | null = null; | ||
|
|
||
| // stable identity so strategies (and classic scheduleOnce dedupe) can | ||
| // coalesce repeat scheduling between flushes | ||
| #revalidateCurrent = (): void => { | ||
| if (this.#renderer !== null) { | ||
| this.revalidate(this.#renderer); | ||
| } | ||
| }; | ||
|
|
||
| scheduleRevalidate(renderer: BaseRenderer): void { | ||
| _backburner.scheduleOnce('render', this, this.revalidate, renderer); | ||
| this.#renderer = renderer; | ||
|
|
||
| const strategy = _getStrategy(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| if (strategy._scheduleRevalidate !== undefined) { | ||
| strategy._scheduleRevalidate(this.#revalidateCurrent); | ||
| } else { | ||
| // a registered strategy without the internal seam leaves the | ||
| // renderer on classic runloop scheduling | ||
| _backburner.scheduleOnce('render', this, this.revalidate, renderer); | ||
| } | ||
| } | ||
|
|
||
| isValid(): boolean { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { _backburner, next as runloopNext, schedule } from '@ember/runloop'; | ||
| import type { Strategy } from '@ember/scheduler'; | ||
|
|
||
| /** | ||
| * The ambient default strategy: schedules exactly the way Ember works | ||
| * today, so existing applications observe no change in timing. Phases | ||
| * map onto the runloop's queues (`render`, then `afterRender`, with | ||
| * `composite` re-scheduled behind `layout`'s queue entries within the | ||
| * same flush), and the renderer's revalidation is a | ||
| * `scheduleOnce('render', ...)`, just as it always was. | ||
| * | ||
| * Render-aware scheduling (frame-aligned phases, coalesced | ||
| * revalidation) is what a swapped-in strategy provides -- see | ||
| * `@ember/scheduler/strategy` -- and becomes the source of performance | ||
| * wins when it becomes the default. | ||
| * | ||
| * @internal | ||
| */ | ||
| class ClassicStrategy implements Strategy { | ||
| render(): Promise<void> { | ||
| return new Promise((resolve) => schedule('render', null, resolve)); | ||
| } | ||
|
|
||
| layout(): Promise<void> { | ||
| return new Promise((resolve) => schedule('afterRender', null, resolve)); | ||
| } | ||
|
|
||
| composite(): Promise<void> { | ||
| return new Promise((resolve) => | ||
| schedule('afterRender', null, () => schedule('afterRender', null, resolve)) | ||
| ); | ||
| } | ||
|
|
||
| next(): Promise<void> { | ||
| return new Promise((resolve) => runloopNext(null, resolve)); | ||
| } | ||
|
|
||
| idle(): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| if (typeof requestIdleCallback === 'function') { | ||
| // fully-idle or backgrounded pages can starve requestIdleCallback | ||
| // indefinitely; cap the wait to keep the promise resolvable | ||
| requestIdleCallback(() => resolve(), { timeout: 500 }); | ||
| } else { | ||
| setTimeout(resolve, 0); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * The renderer's internal seam: how revalidation gets scheduled. | ||
| * Classic behavior is a runloop `scheduleOnce`, preserving today's | ||
| * timing exactly (the flush callback is stable per renderer, so | ||
| * scheduleOnce's dedupe applies as before). | ||
| */ | ||
| _scheduleRevalidate(flush: () => void): void { | ||
| _backburner.scheduleOnce('render', null, flush); | ||
| } | ||
| } | ||
|
|
||
| const classicStrategy: ClassicStrategy = new ClassicStrategy(); | ||
|
|
||
| export default classicStrategy; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { assert } from '@ember/debug'; | ||
| import classicStrategy from '@ember/scheduler/-private/classic'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should not import this here. |
||
|
|
||
| /** | ||
| The `@ember/scheduler` package provides a render-aware scheduling interface, | ||
|
|
@@ -67,6 +68,18 @@ export interface Strategy { | |
| composite(): Promise<void>; | ||
| next(): Promise<void>; | ||
| idle(): Promise<void>; | ||
|
|
||
| /** | ||
| * Internal seam used by the renderer to schedule revalidation. The | ||
| * public phase functions are for user work; revalidation is hotter | ||
| * than any user phase, so the renderer talks to the strategy through | ||
| * this callback-based hook rather than allocating promises per | ||
| * invalidation. Optional: strategies that do not implement it leave | ||
| * the renderer on its classic runloop scheduling. | ||
| * | ||
| * @internal | ||
| */ | ||
| _scheduleRevalidate?(flush: () => void): void; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this here? we can't add to the interface of the scheduler, even for compat |
||
| } | ||
|
|
||
| let registeredStrategy: Strategy | null = null; | ||
|
|
@@ -123,12 +136,20 @@ export function _clearRegisteredStrategy(): void { | |
| registeredStrategy = null; | ||
| } | ||
|
|
||
| function getStrategy(phaseName: string): Strategy { | ||
| assert( | ||
| `Attempted to schedule work into the '${phaseName}' phase, but no scheduling strategy is registered. Register a strategy when defining your Application, e.g. the default strategy:\n\n\timport { registerStrategy } from '@ember/scheduler';\n\timport strategy from '@ember/scheduler/strategy';\n\n\tregisterStrategy(strategy);`, | ||
| registeredStrategy !== null | ||
| ); | ||
| return registeredStrategy; | ||
| function getStrategy(): Strategy { | ||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took exactly this direction in df40e9e: no ambient fallback anywhere — the glimmer<->ember hookup ( |
||
| } | ||
|
|
||
| /** | ||
| * The renderer's accessor for the active strategy. | ||
| * | ||
| * @internal | ||
| */ | ||
| export function _getStrategy(): Strategy { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function is redundant and unneeded. don't be silly
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gone — |
||
| return getStrategy(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -156,7 +177,7 @@ function getStrategy(phaseName: string): Strategy { | |
| @public | ||
| */ | ||
| export function render(): Promise<void> { | ||
| return getStrategy('render').render(); | ||
| return getStrategy().render(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why call getStrategy so much when we can just directly access registeredStrategy?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed and removed: |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -182,7 +203,7 @@ export function render(): Promise<void> { | |
| @public | ||
| */ | ||
| export function layout(): Promise<void> { | ||
| return getStrategy('layout').layout(); | ||
| return getStrategy().layout(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -212,7 +233,7 @@ export function layout(): Promise<void> { | |
| @public | ||
| */ | ||
| export function composite(): Promise<void> { | ||
| return getStrategy('composite').composite(); | ||
| return getStrategy().composite(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -237,7 +258,7 @@ export function composite(): Promise<void> { | |
| @public | ||
| */ | ||
| export function next(): Promise<void> { | ||
| return getStrategy('next').next(); | ||
| return getStrategy().next(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -261,5 +282,5 @@ export function next(): Promise<void> { | |
| @public | ||
| */ | ||
| export function idle(): Promise<void> { | ||
| return getStrategy('idle').idle(); | ||
| return getStrategy().idle(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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