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

Slower Rendering/Filtering with 10,000+ Nodes #62

Closed
nbergqui opened this issue Jul 16, 2015 · 9 comments
Closed

Slower Rendering/Filtering with 10,000+ Nodes #62

nbergqui opened this issue Jul 16, 2015 · 9 comments

Comments

@nbergqui
Copy link
Contributor

I am seeing initial render times of 7-12 seconds (depending on browser) and filter keypress delays of 1 or so second with a 10,000 (ish) node tree. This is without using any custom templates and only setting the id, label, and children attributes.

As a workaround, I changed the ng-if in the ivhTreeviewChildren directive from "getChildren().length" to "trvw.isExpanded(node)". This seems to delay the rendering of a nodes children until toggleExpand gets called and brings the rendering and filtering performance back to instantaneous. Unfortunately this also breaks some of the tests which expect all children to be rendered on initial load.

@robwalkerco
Copy link

Hi @nbergqui. I've also just noticed the high numbers of watchers on the tree and massively reduced it by supplying a custom template (https://github.com/iVantage/angular-ivh-treeview#tree-node-templates) with -
<div ivh-treeview-children ng-if="trvw.isExpanded(node)"></div>

You don't need to edit the directives themselves then 😄

You can also remove title="{{trvw.label(node)}}" and ng-if="trvw.useCheckboxes()" from the template if you don't require them to further reduce the watchers and hence speed up the digest cycle.

@jtrussell
Copy link
Contributor

That's an interesting optimization. I suppose it would add some overheard when the tree is expanded but probably all upside in practice. We'll take a look at refactoring the tests - they could use some cleanup as it is - and I'll have to think through whether there's any reason it would make sense to have all the nodes rendered by default.

Also, If you're using a version of angular that supports it you can likely squeeze out a fair bit of performance by using the bind-once syntax {{:: thing }} everywhere in a custom template.

@nbergqui
Copy link
Contributor Author

Hello @papertrailrob. Just using a custom template is better than editing the directive. Thanks for the suggestion ... I need to explore custom templating more.

I did notice that just setting the ng-if of treeview-children to trvw.isExpanded(node) caused some other unexpected behavior. The expandToDepth option seemed to override the expand state of any expanded nodes when the top node of the tree was collapsed and then expanded again.

I'm not sure that this is a good solution, but I ended up adding a new option, renderChildrenOnExpand, to opt in to this feature and adding a new collection item attribute, renderChildrenAttribute, to track which nodes have been expanded. This allows the current tests to execute successfully and seems to fix the other unexpected behavior. I pushed these changes to my fork of this directive: https://github.com/nbergqui/angular-ivh-treeview.

@jtrussell This is a great directive. Thanks for all the effort!

@jtrussell
Copy link
Contributor

@nbergqui Glad you find it useful!

Sounds like we can close this for now but feel free to reopen if needed.

I'm going to tag the v1 milestone issue (#27) seems like something along these lines could be reasonable to get in for that release. Let us know how your changes work out!

@fhurta
Copy link
Contributor

fhurta commented Sep 13, 2017

I'm dealing with 10K nodes in treeview and I tried custom template with bind-once syntax, removed the chcek ng-if="trvw.useCheckboxes()" (as I always use the checkboxes) but it still was not enough.
I tried @nbergqui 's clone with his new option and it works well. There are just few changes in source code and it seems he keeps his clone updated with latest changes.

Guys @jtrussell , @nbergqui ,
How about incorporating the changes into this library?

@nbergqui
Copy link
Contributor Author

Hi @fhurta, I'm happy to make another pull request. What do you think @jtrussell?

I still regularly use this tree control and try to bring my fork up to date every so often. Just today, I've been working on rendering a tree that could have up to 150k nodes.

@jtrussell
Copy link
Contributor

I'd love to update the default template to behave this way. Unfortunately, I think it will break the majority of our test cases. If you can implement this change without breaking the build 👍 .

@nbergqui
Copy link
Contributor Author

Yeah, I left the default template as is and added a new option (renderChildrenOnExpand) to implement this functionality. Probably less than ideal, but it works for me ... and all the test cases pass.

@fhurta
Copy link
Contributor

fhurta commented Sep 14, 2017

@jtrussell Not sure if you had time to look at the changes by @nbergqui but it is basically what you address in #190 (not rendering collapsed chidren) only it is not in the template but is driven by the (opt-in) option in code.

The solution you described in #190 involves bigger changes that probably deserves new version (v2). But meanwhile I would see adding the option as useful enhancement of v1.

Thanks for considering

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

No branches or pull requests

4 participants