😪 Make `view.childViews` lazy #14139

chancancode · 2016-08-27T08:44:12Z

🆕 getRootViews returns an array of “top-level” components
🆕 getChildViews to replace view.childViews (to be deprecated 🔜)
🆕 “tagless” components are registered in the view-registry with a GUID
🆒 view.childViews is now a getter that delegates to getChildViews
🆓 view.childViews no longer leak destroyed components (bug in beta)
🔙 view.childViews now tracks updates (components added or removed during updates) correctly, which was unreliable since 1.13

Instead of tracking childView instances, we now track a list of IDs that can be used to look them up in the view registry. This has the benefit of not holding a reference to the view instance – thus do not require cleanup when the child views are destroyed.

When requested, we lazily query the view registry to reify the (still) live children and cleanup and stale IDs while we are at it.

This approach does leak the string IDs of destroyed components, however the effect is believed to be minimal. If this ended up causing problems, we can be smarter about this, such as tracking the number of removals and schedule a collection (outside of the render loop) when a view has accumulated enough removals.

chancancode · 2016-08-27T08:44:25Z

cc @teddyzeenny

praxxis · 2016-08-27T14:28:23Z

packages/ember-metal/tests/descriptor_test.js

+    }
+  },
+
+class extends DescriptorTest {


indentation seems off here

chancancode · 2016-08-27T16:57:30Z

packages/ember-glimmer/lib/syntax/curly-component.js

@@ -188,7 +188,6 @@ class CurlyComponentManager {
    aliasIdToElementId(args, props);

    props.parentView = parentView;
-    props.renderer = parentView.renderer;


The last failure is because I removed this code. It seems like engine components are not getting these injections setup: https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application-instance.js#L291-L297 (maybe the rest of things in that block too?)

I can add this back, but it seems better to just fix the injection. @rwjblue @dgeb can you halp?

Ya, I can fix. Might be easier to put it back for now and I'll fix with a follow up PR later this afternoon...

Tackled this in #14140 (which has caused you yet another rebase conflict). Sorry...

homu · 2016-08-28T02:25:07Z

☔ The latest upstream changes (presumably #14140) made this pull request unmergeable. Please resolve the merge conflicts.

This is dead code, we don’t actually call these methods anywhere in the codebase anymore.

We also don’t need to propagate `parentView` in `linkChild` anymore: since 9228bb8, `parentView` is supplied via the “create props” for both engines to ensure that it is available during `init`.

🆕 `getRootViews` return an array of “top-level” components 🆕 `getChildViews` to replace `view.childViews` (to be deprecated 🔜) 🆕 “tagless” components are registered in the view-registry with a GUID 🆒 `view.childViews` is now a getter that delegates to `getChildViews` 🆓 `view.childViews` no longer leak destroyed components (bug in beta) 🔙 `view.childViews` now tracks updates (components added or removed during updates) correctly, which was unreliable since 1.13 Instead of tracking `childView` instances, we now track a list of IDs that can be used to look them up in the view registry. This has the benefit of not holding a reference to the view instance – thus do not require cleanup when the child views are destroyed. When requested, we lazily query the view registry to reify the (still) live children and cleanup and stale IDs while we are at it. This approach does leak the string IDs of destroyed components, however the effect is believed to be minimal. If this ended up causing problems, we can be smarter about this, such as tracking the number of removals and schedule a collection (outside of the render loop) when a view has accumulated enough removals.

chancancode · 2016-08-28T07:34:27Z

Fixed the IE9 errors – they are due to IE9 not supporting strict mode

chancancode · 2016-08-28T07:35:27Z

Confirmed no regression: https://cdn.rawgit.com/chancancode/15c2b63ea158bc704b88758bd401a7be/raw/f387a5ef50aa9eed5e0a34bf39e2b9c9fddb9eb3/report.html

chancancode · 2016-08-28T07:40:47Z

To be clear – the "confirmed no regression" means normal initial render performance, as opposed to .childView performance. It is expected that .childView will be slower, but since it's used for debugging we believe that is an acceptable tradeoff. This is better than the alternative where we have to pay the bookkeeping cost whenever we teardown a component during normal rendering.

chancancode · 2016-08-28T07:43:42Z

packages/ember-views/lib/mixins/child_views_support.js

  },

  linkChild(instance) {
-    if (!instance[OWNER]) {
+    if (!getOwner(instance)) {
      setOwner(instance, getOwner(this));
    }


@rwjblue @dgeb as a follow-up, do you think we still need this?

No, likely not needed.

teddyzeenny · 2016-08-28T09:56:49Z

Shouldn't getRootViews and getChildViews be exposed on the ViewUtils object here?

teddyzeenny · 2016-08-28T10:10:22Z

Updated in #14143

chancancode force-pushed the lazy-child-views branch from 3e2f1a3 to 5426dcb Compare August 27, 2016 09:13

praxxis reviewed Aug 27, 2016
View reviewed changes

packages/ember-metal/tests/descriptor_test.js

}

},

class extends DescriptorTest {

Copy link

Contributor

praxxis Aug 27, 2016

Choose a reason for hiding this comment

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

indentation seems off here

chancancode force-pushed the lazy-child-views branch from 5426dcb to e737292 Compare August 27, 2016 16:55

chancancode reviewed Aug 27, 2016
View reviewed changes

chancancode force-pushed the lazy-child-views branch from e737292 to 52edd7a Compare August 27, 2016 17:23

rwjblue mentioned this pull request Aug 27, 2016

[BUGFIX beta] Ensure injections happen in engine instances. #14140

Merged

chancancode added 6 commits August 27, 2016 20:41

Use a dictionary for the view registry

d78c0dd

[CLEANUP] Remove childViews cleanup logic

6fc0790

This is dead code, we don’t actually call these methods anywhere in the codebase anymore.

[CLEANUP] Glimmer 2 does not need ownerView

cb429a0

We also don’t need to propagate `parentView` in `linkChild` anymore: since 9228bb8, `parentView` is supplied via the “create props” for both engines to ensure that it is available during `init`.

[CLEANUP] Make ApplicationTest have runTask

5840266

Introduce a way to make ES5 getters from Mixins

48e52b3

chancancode force-pushed the lazy-child-views branch from 52edd7a to 4e5ddae Compare August 28, 2016 03:42

IE9 doesn’t seem to be fully spec-compliant here

8f3c5b5

chancancode force-pushed the lazy-child-views branch from f95c9c9 to 8f3c5b5 Compare August 28, 2016 07:09

chancancode merged commit 8a49ddd into master Aug 28, 2016

chancancode deleted the lazy-child-views branch August 28, 2016 07:41

chancancode reviewed Aug 28, 2016
View reviewed changes

chancancode mentioned this pull request Aug 29, 2016

Do we need to warm guidFor? #14152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

😪 Make `view.childViews` lazy #14139

😪 Make `view.childViews` lazy #14139

chancancode commented Aug 27, 2016 •

edited

Loading

chancancode commented Aug 27, 2016

praxxis Aug 27, 2016

chancancode Aug 27, 2016

rwjblue Aug 27, 2016

chancancode Aug 27, 2016

rwjblue Aug 28, 2016

homu commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode Aug 28, 2016

rwjblue Aug 28, 2016

teddyzeenny commented Aug 28, 2016

teddyzeenny commented Aug 28, 2016

😪 Make view.childViews lazy #14139

😪 Make view.childViews lazy #14139

Conversation

chancancode commented Aug 27, 2016 • edited Loading

chancancode commented Aug 27, 2016

praxxis Aug 27, 2016

Choose a reason for hiding this comment

chancancode Aug 27, 2016

Choose a reason for hiding this comment

rwjblue Aug 27, 2016

Choose a reason for hiding this comment

chancancode Aug 27, 2016

Choose a reason for hiding this comment

rwjblue Aug 28, 2016

Choose a reason for hiding this comment

homu commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode commented Aug 28, 2016

chancancode Aug 28, 2016

Choose a reason for hiding this comment

rwjblue Aug 28, 2016

Choose a reason for hiding this comment

teddyzeenny commented Aug 28, 2016

teddyzeenny commented Aug 28, 2016

😪 Make `view.childViews` lazy #14139

😪 Make `view.childViews` lazy #14139

chancancode commented Aug 27, 2016 •

edited

Loading