Skip to content
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

Render Aware Scheduler Interface #957

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Sep 9, 2023

Rendered

This RFC Proposes replacing @ember/runloop (Backburner.js) with an interface for common scheduling needs.

The interface describes intent for when work should be performed in relation to the native event queues and render cycle of the browser. The details of how that work is scheduled and flushed are up to the specific scheduler implementation, allowing for experiments in this space.

Additionally, this RFC proposes deprecations and alterations to associated async primitives such as RSVP to support this exploration.

@runspired runspired added T-ember-data RFCs that impact the ember-data library T-deprecation T-routing T-framework RFCs that impact the ember.js library T-testing T-fastboot RFCs that impact the fastboot library T-learning T-ember-cli RFCs that impact the ember-cli library S-Exploring In the Exploring RFC Stage labels Sep 9, 2023
@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Sep 9, 2023
@runspired runspired self-assigned this Sep 9, 2023
@runspired runspired removed the S-Proposed In the Proposed Stage label Sep 9, 2023
@kategengler
Copy link
Member

Moving to the Exploring stage requires the consensus of the relevant teams. See :https://github.com/emberjs/rfcs#exploring I am removing the label.

@kategengler kategengler added S-Proposed In the Proposed Stage and removed S-Exploring In the Exploring RFC Stage labels Sep 10, 2023
@NullVoxPopuli
Copy link
Contributor

What are the SSR impacts? (if we were to re-invest SSR in the most ideal way)

  • what is settled?
  • ignore idle? do we ignore it?
    -> flush idle queue?
    -> ensure flushed?
  • how to test with idle?

if you have a lot of timings / queues, how do you prevent folks from just arbitrarily adding things to whatever the "next queue" is?


how do we suggest / teach when folks will want to use one queue over others?

@wagenet wagenet added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Sep 22, 2023
@wagenet
Copy link
Member

wagenet commented Sep 22, 2023

Given that @ef4 and @wycats are generally in favor of this direction we agreed at the RFC review meeting to move to Exploring.

@wycats
Copy link
Member

wycats commented Sep 23, 2023

Thumbs up. We got into the details in the spec meeting and I think this is absolutely ready for Exploring.

@ef4
Copy link
Contributor

ef4 commented Oct 13, 2023

Status update here: this is looking good, @runspired is leading experimentation in the implementation side as the next action before putting this forward as formal public API.

@runspired
Copy link
Contributor Author

runspired commented Mar 1, 2024

As an update: I ran AuditBoard's test suite replacing the RSVP flush with a native microtask instead of flushing in the runloop, and EmberData 4.12.x (which drops all RSVP and almost all runloop utilization). A few observations:

EmberData 4.12.x upgrade itself had a few timing things to work out, but it didn't take us too long to get through them despite ~12k tests, and they all fell into a bucket of bugs that wouldn't exist with the proposed scheduler.

RSVP: changing to flush with a microtask failed only 3 tests, all for an identical cause:
In a route afterModel we had this:

scheduleOnce('afterRender', this, () => {
  this.send('onSetBreadcrumb');
});

With RSVP flushing as a native microtask, this now runs after instead of before the completion handling of many promises associated with the transition. Were we to adopt this proposal though, this would have been a non-issue. It was also 1 shared cause for 3 test failures out of 12k tests that proved easy to fix.

I next tried removing our reliance on @ember/jquery to avoid its wrapping of xhr resolutions into the runloop. We rely on this because we set useFetch: false in all of our adapters. Notably: the default for EmberData has for all of 4.x been useFetch: true.

This resulted in dozens of test failures, possibly as many as a hundred. In debugging, it appears the reason for these failures is that tests that triggered async actions and ember-concurrency tasks were relying on the runloop initiated by jquery to count towards settledness. ember-concurrency itself makes widespread use of the runloop for most task related work. Combined, this means that await settled() and await render() and await click() and similar in tests capture a large body of async that might otherwise escape.

However, fixing this is as simple as ember-concurrency adding its own test waiters. EmberData already uses one, but there are some scenarios it intentionally does not cover (background requests). In these tests the cause was the request was being kicked off several promises deep from the triggering action and thus escaped the bounds of settled due to the missing EC waiter. The reason the jquery runloop usage papered over this was that it joined the next EC tick flush to the promise resolution in the test. Even if EC did not add a waiter, this is a class of issues that would not exist if we were to adopt the proposed RFC because the associated EmberData waiter would have been properly captured despite being multiple promises deep.

Current conclusions:

  • I see no significant blocker to continuing to explore this RFC
  • The biggest risk seems to be to tests not app code, and most prominently in locations where EC is in use
  • We should shore up EC before shipping a new scheduler to avoid friction there
  • apps using useFetch: false should really stop doing so ;)

Additional design note: it might be a nice thing to wrap the functions passed to the on modifier in tests with a waiter if they return a promise. This could avoid a lot of test-waiter creation.

@ef4
Copy link
Contributor

ef4 commented Mar 8, 2024

Feedback from RFC review discussion:

  • let's add detail to "how we teach this" explaining exactly what is deprecated and how it should appear in the deprecation guide and what people should be told about replacements
  • what's the experience in terms of when people will see deprecations, when should they flip the optional feature flag.
  • what is the rollout timeline? Which deprecations until 6.0? 7.0?


We divide this work into 6 conceptual phases:

- `tasks` - work that updates reactive state
Copy link
Contributor

Choose a reason for hiding this comment

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

we may need a sync phase between tasks and render

this phase would be before we try rendering.

e.g.:

  • sync runs before render
    • and after component construction
    • not in the render tracking frame / does not invalidate the render tracking frame
  • must not run when the parent context is torn down

our purpose for on.sync is to create a mechanism for constructing a stateful object with a lifetime ("resource") that can use reactive data as part of its constructor. when the stateful data changes, the resource is cleaned up and reconstructed. also, when the parent lifetime is destroyed, the resource is cleaned up. and then finally, the resource object itself has a stable identity regardless of whether it has the internal resource has been synchronized or reset.

tl;dr: stable object on the outside, unstable object on the inside (which gets reset based on reactivity)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Exploring In the Exploring RFC Stage T-deprecation T-ember-cli RFCs that impact the ember-cli library T-ember-data RFCs that impact the ember-data library T-fastboot RFCs that impact the fastboot library T-framework RFCs that impact the ember.js library T-learning T-routing T-testing
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants