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

[WIP] Make model hooks run in parallel #12415

Closed
wants to merge 1 commit into from

Conversation

nickiaconis
Copy link
Contributor

Opening for discussion on how to support running model hooks in parallel. From an IRL discussion with @wycats and @nathanhammond, this should be built as an addon, so that apps must explicitly opt into the functionality and related consequences. I am laying out the required changes directly in the source so we can plan how to hook into the necessary parts of Ember from that addon.

The big consequence of making model hooks run in parallel is that modelFor must return a promise. Here are the changes that make that happen.

Parallel model hooks require changes to router.js as well: tildeio/router.js#171

@rwjblue
Copy link
Member

rwjblue commented Sep 30, 2015

Making the model hook sync and changing the return value of modelFor definitely seem like a breaking changes, am I missing something? Do these public API changes correspond with an RFC?

@ef4
Copy link
Contributor

ef4 commented Oct 1, 2015

I found the description of this issue misleading. "Synchronous" in most of javascript means "returning a value immediately", which is not the case here.

Perhaps it would be better to say "make model hooks run in parallel".

@nickiaconis
Copy link
Contributor Author

@rwjblue -- "I am laying out the required changes directly in the source so we can plan how to hook into the necessary parts of Ember from that addon."

I'm using these two PRs basically as glorified diffs so a discussion may be had regarding what hooks are needed in Ember to enable an addon to achieve this same result.

@rwjblue
Copy link
Member

rwjblue commented Oct 1, 2015

Random thoughts:

  • I would prefer to avoid "secret handshake" mode. I do not like to have a special "turbo mode" that is not obvious and/or default behavior for most users.
  • Using the same API (Ember.Route#modelFor) to either return the final value or a promise depending on if another external addon is present seems like it would a massive troll factor. This could be resolved by using a different method name for the promise returning version, but keeping the current modelFor around means that we likely can't address the parallel case (which I assume is the point).
  • I am not a fan of addons modifying Ember internal behaviors. This leads to super confusing errors that users have no idea how to debug.

@nathanhammond
Copy link
Member

We've trained an army of Ember developers to expect that routes and models resolve in a particular manner but it turns out that it's a tremendous performance anti-pattern. We have examples in our application where an approach like this would reduce page load time by 500ms or more and also allows for a much better incremental loading behavior. With that much of a win it's not optional for us to skip this as an optimization. (@nickiaconis is working on posting a side-by-side demo.)

Further, our performance wins imply that this approach would be just as beneficial for other members of the Ember community. If it weren't for the existing API I feel like everybody adopting this behavior would be a no-brainer. That being said, we likely can't drop support for our present pattern for approximately two years.

Ember's options for landing behavior like this, ranked in order of my preference for our application:

  1. Tie this paradigm change to something opt-in like routable components where people will already have to change their behavior.
  2. Maintain it behind a feature flag on the Canary branch which you can toggle on and off via config until we can land it.
  3. Provide some built-in hooks so that we're able to reach in and change this behavior as an addon in a safer way.
  4. The worlds most absurd monkeypatch–which would also be packaged as an addon.
  5. Maintain a patch and build our own custom Ember.

Let's skip talking about exact implementation for a moment (e.g. modelForPromise) and focus on the concept and how we as a community would prefer to handle it. If we elect for one of the first two approaches we would present this as an RFC. If we want three or four we'd still like to work closely with the Ember core team since even as an addon it's likely to be problematic: if people show up on Slack to ask for performance help a default recommendation might become "go install this addon and make your app support it."

@ef4
Copy link
Contributor

ef4 commented Oct 1, 2015

Let's skip talking about exact implementation for a moment (e.g. modelForPromise) and focus on the concept and how we as a community would prefer to handle it.

It is 100% clear to me that this needs to be an RFC, for precisely the reasons you highlight. If you need close coordination with Ember, and if we need a coherent story for the community, then we need to do it right and have a coherent plan.

I am totally on board with this feature and have had several discussions in the past with other core team members who also want this feature. There will be no barrier to getting it done, it just needs to be fully designed in an RFC.

@nickiaconis nickiaconis changed the title [WIP] Make model hooks run synchronously [WIP] Make model hooks run in parallel Oct 1, 2015
@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2016

Closing this PR in favor of the discussion and progress in emberjs/rfcs#97

@rwjblue rwjblue closed this Apr 10, 2016
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.

4 participants