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

[FEATURE] Removes Chains in Tracked Properties Flag #17951

Merged
merged 1 commit into from Apr 26, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 18, 2019

This PR updates the tracked properties feature flag to fully remove
chains. This was primarily is response to core issues we found with
interop with computed properties, which required us to make computed
properties and observers more lazy in general.

At a high level, these are the major changes:

  • The getCurrentTracker/setCurrentTracker system was a leaky
    abstraction, since errors could cause a child tracker to never reset.
    In order to make it more bulletproof, it has been changed to
    track/consume/isTracking. We wrap any change to the stack of
    trackers with a try/catch to make sure we always clean up in
    track. isTracking is used only when we don't want to eagerly
    create a tag if there is no tracking context.
  • Observers have been made asynchronous. They now flush during specific
    phases of the runloop - just before render, and potentially after
    render if anything was scheduled (this will begin a new runloop).
    They poll now to check if any changes occured, rather than firing
    synchronously when the change occurs, which is how we made them work
    entirely using tags without chains. Observers inherited from
    EmberObject classes begin polling at the same time as
    finalizeChains.
  • Computed properties have been updated to only use chains when
    checking for dirtiness. Since computed properties were lazy before,
    this isn't much of a change overall. Computed properties also no
    longer autotrack, unless they have been marked by a (currently
    private) _auto() modifier.
  • Both observers and computeds accomplish this laziness by following and
    reading the tags of their dependencies after calculation. They
    entangle all dependencies at this point, unless the dependency is an
    uncalculated computed property. If they encounter one of these, they
    setup lazy chains, which will be followed and updated the next time
    the computed property is calculated. This also makes aliases lazily
    observe.
  • Computed properties have also been updated to install a native setter,
    per the track props update RFC.
  • Query parameters use synchronous observers to update various things.
    They should really be refactored, but that's going to take a while.
    In the meantime, we flush the observers synchronously for them
    specifically.
  • A UNKNOWN_PROPERTY_TAG system has been added privately and
    internally only
    . This system allows proxies to return a special tag
    that invalidates when their content's properties change. This system
    could be made more public in the future, but it is purposefully
    private for the time being. It was necessary to match existing
    semantics in many tests.
  • A new mandatory setter system has been added. This system is now
    one-way - once a value has been consumed, there is no way to remove
    the setter and "unconsume" it. This is the nature of tags being lazy
    and having no teardown.

There were a few additional changes that were required as well:

  • visit and transitionTo for application tests had to be made
    async in order to work properly with observers. Most of the work
    occured in another PR, but some had to be finished up here.
  • Many, many tests needed to be updated. Most of these were for the fact
    that observers are now async, and required us to wait on the runloop
    settling.

TODO:

  • Performance analysis and tuning
  • Add test for native CP setter
  • Add tests for new Mandatory Setter (this is already tested indirectly and directly in some ways, so may not be necessary to merge)

@pzuraq pzuraq force-pushed the feature/remove-chains-from-tracked-world branch from b91752f to 97b5d28 Compare April 19, 2019 23:01
@pzuraq pzuraq force-pushed the feature/remove-chains-from-tracked-world branch 3 times, most recently from 8ce9706 to 6cb5921 Compare April 25, 2019 20:18
@pzuraq pzuraq changed the title [WIP][FEATURE] Remove chains from tracking [FEATURE] Removes Chains in Tracked Properties Flag Apr 25, 2019
@pzuraq pzuraq force-pushed the feature/remove-chains-from-tracked-world branch 6 times, most recently from 33e4200 to 4cdbb8c Compare April 26, 2019 04:48
This PR updates the tracked properties feature flag to fully remove
chains. This was primarily is response to core issues we found with
interop with computed properties, which required us to make computed
properties and observers more lazy in general.

At a high level, these are the major changes:

* The `getCurrentTracker`/`setCurrentTracker` system was a leaky
  abstraction, since errors could cause a child tracker to never reset.
  In order to make it more bulletproof, it has been changed to
  `track`/`consume`/`isTracking`. We wrap any change to the stack of
  trackers with a `try/catch` to make sure we always clean up in
  `track`. `isTracking` is used only when we don't want to eagerly
  create a tag if there is no tracking context.
* Observers have been made asynchronous. They now flush during specific
  phases of the runloop - just before render, and potentially after
  render if anything was scheduled (this will begin a new runloop).
  They poll now to check if any changes occured, rather than firing
  synchronously when the change occurs, which is how we made them work
  entirely using tags without chains. Observers inherited from
  EmberObject classes begin polling at the same time as
  `finalizeChains`.
* Computed properties have been updated to only use chains when
  checking for dirtiness. Since computed properties were lazy before,
  this isn't much of a change overall. Computed properties also no
  longer autotrack, unless they have been marked by a (currently
  private) `_auto()` modifier.
* Both observers and computeds accomplish this laziness by following and
  reading the tags of their dependencies _after calculation_. They
  entangle all dependencies at this point, _unless_ the dependency is an
  uncalculated computed property. If they encounter one of these, they
  setup _lazy chains_, which will be followed and updated the next time
  the computed property is calculated. This also makes aliases lazily
  observe.
* Computed properties have also been updated to install a native setter,
  per the track props update RFC.
* Query parameters use synchronous observers to update various things.
  They should really be refactored, but that's going to take a while.
  In the meantime, we flush the observers synchronously for them
  specifically.
* A `UNKNOWN_PROPERTY_TAG` system has been added _privately and
  internally only_. This system allows proxies to return a special tag
  that invalidates when their _content's_ properties change. This system
  could be made more public in the future, but it is purposefully
  private for the time being. It was necessary to match existing
  semantics in many tests.
* A new mandatory setter system has been added. This system is now
  one-way - once a value has been consumed, there is no way to remove
  the setter and "unconsume" it. This is the nature of tags being lazy
  and having no teardown.

There were a few additional changes that were required as well:

* `visit` and `transitionTo` for application tests had to be made
  async in order to work properly with observers. Most of the work
  occured in another PR, but some had to be finished up here.
* Many, many tests needed to be updated. Most of these were for the fact
  that observers are now async, and required us to wait on the runloop
  settling.
@pzuraq pzuraq force-pushed the feature/remove-chains-from-tracked-world branch from 4cdbb8c to 405d423 Compare April 26, 2019 14:51
@rwjblue
Copy link
Member

rwjblue commented Apr 26, 2019

Discussed in today's Ember core team meeting, and we should land this as soon as we confirm that when the feature is disabled, there is no performance regression. We can iterate (on master) to fix any performance regressions when the feature is enabled...

@rwjblue
Copy link
Member

rwjblue commented Apr 26, 2019

@rwjblue
Copy link
Member

rwjblue commented Apr 26, 2019

Looks like this is fine with the flag off, so I'm going to go ahead and merge this.

@rwjblue rwjblue merged commit 91656e1 into master Apr 26, 2019
@rwjblue rwjblue deleted the feature/remove-chains-from-tracked-world branch April 26, 2019 22:14
@GavinJoyce
Copy link
Member

GavinJoyce commented May 2, 2019

Observers have been made asynchronous. They now flush during specific
phases of the runloop - just before render, and potentially after
render if anything was scheduled (this will begin a new runloop).

Might this be a breaking change?

https://api.emberjs.com/ember/3.9/classes/Observable/methods/incrementProperty?anchor=set

Screen Shot 2019-05-02 at 14 08 10

As an example, this change has broken some helpers in ember-composable-helpers: https://github.com/DockYard/ember-composable-helpers/issues/318

@rwjblue
Copy link
Member

rwjblue commented May 2, 2019

@GavinJoyce yep, definitely might be, all work was done behind a feature flag so that we can test and experiment the impact

@GavinJoyce
Copy link
Member

Thanks, I'm going to try a custom ember-souce build on Intercom to see what breaks. We have ~200 observers in older parts of our app. I'm happy to migrate away from them if necessary, but it would be pretty sad if we couldn't upgrade to Octane first and then incrementally migrate to tracked properties.

@pzuraq
Copy link
Contributor Author

pzuraq commented May 2, 2019

@GavinJoyce FWIW, the only semi-common use case we found where sending observers synchronously was really required was if you were using them to invalidate a computed property, and we have a way to mitigate that using tracked properties. That said, you could definitely see how this could cause issues timing-wise in apps with heavy observer usage, so your feedback would definitely be valuable!

Unfortunately, this is the only way we could make observers interop with tracked properties, since they're fundamentally lazy, so if we this is too problematic and we do have to revert it, it means we'll also have to drop that interop 😕

@pzuraq
Copy link
Contributor Author

pzuraq commented May 29, 2019

@GavinJoyce we just merged an update that brings back synchronous observers, and adds the ability to opt-into async observers. Can you let us know if your apps are able to work with the latest changes? We need testers here to make sure things are solid before we ship, definitely appreciate your input! 😄

@GavinJoyce
Copy link
Member

GavinJoyce commented May 30, 2019

Thanks @pzuraq, we've just upgraded our app to 3.10 so I'm going to spend some time next week working with 3.11.beta and canary. I'll also upgrade the Octane app that I'm building for a future tutorial and try it with ember-composable-helpers. I'll report back once I have some results. Thanks again

@GavinJoyce
Copy link
Member

I've confirmed that https://github.com/DockYard/ember-composable-helpers/issues/318 is no longer an issue with ember canary. I'll currently addressing issues with our Intercom app in 3.11.0-beta.1, I'll report back again once I've managed to run Intercom on canary and played with the observer options

@GavinJoyce
Copy link
Member

GavinJoyce commented May 31, 2019

I also tried canary with our internal composer addon, this is a pretty complex addon which has remained pretty static for the past 3 years. It uses ~50 observers and has a comprehensive test suite. CI is 🍏 👍:

Screen Shot 2019-05-31 at 12 25 17

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.

None yet

4 participants