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

Watch values rather than functions #69

Open
jtrussell opened this issue Aug 27, 2015 · 3 comments
Open

Watch values rather than functions #69

jtrussell opened this issue Aug 27, 2015 · 3 comments

Comments

@jtrussell
Copy link
Contributor

Watching functions is slow, and we do it often.

@jtrussell jtrussell added this to the v1.x - Optimize milestone Aug 27, 2015
@icfantv
Copy link

icfantv commented Mar 17, 2016

What would you think about making the directive responsible for adding/removing a class based on an event (click) rather than using watches to be notified of when a node changes which then set a property on the node (when then have an ng-class to assign the value? This would remove two watches per node?

We have a large tree of television networks (973 leaf nodes) such that affiliates are grouped by parent - e.g., the ABC parent node will have 28 children nodes. With this setup, the default tree creates 13,563 watches. Overriding the node template and using one-time binding syntax drops it to 10,452. Adding an ng-if to the ivh-treeview-children <div> actually creates more watches (63 in our case) but I can't speak to the performance tradeoff mentioned in your documentation.

Tree traversal is at most an O(n) operation assuming you only visit each node once. I need to learn how your directives are structured better to see if we can leverage the observer pattern on parent directives to provide this functionality. But this should reduce the number of watches SIGNIFICANTLY - with the expense of visiting each tree node once.

Thoughts?

@jtrussell
Copy link
Contributor Author

Could definitely be interesting. IIRC there are maybe 15 watchers/node with the default template. Cutting out a handful of watchers per node won't buy you tons of headroom but it certainly won't hurt. At some level just using ng-repeat internally and the fact that each node has an isolate scope is going to be a bottleneck. In a prior life this module used an evented system for managing updates but we haven't tried implementing a more explicit observer pattern, I'd be all ears if you have suggestions though I'd really love to get #84 finished before too much optimization happens.

I'm really surprised that adding the ng-if increases your watch count. If I'm remembering the watchers/node ratio right that would mean you typically have at least ~93% of your nodes visible at a given time, does that sound about right?

@icfantv
Copy link

icfantv commented Mar 18, 2016

I looked at #62 and had applied the wrong controller method to the ivh-treeview-children node via ng-if - using the right method helps significantly, 4400 watches on initial load. We have the networks split into Broadcast and On Demand w/ the Broadcast tree open by default. It does have the bulk of the networks so that would explain high number of initial watches. Your number sounds about right, if a little high - I am using a custom node template and have done some more optimizations (it's a CB tree, so I don't need that ng-if, for example).

I would agree that using events is not the right way to go and you're right in that just building the DOM up for a large tree is the bottleneck. I'm wondering if using the shadow DOM is the right way to go here to speed things up and separate the styling from the DOM structure...or if it would make a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants