angular-tree-component icon indicating copy to clipboard operation
angular-tree-component copied to clipboard

Slow preformance on expand and select checkboxes

Open mojo2405 opened this issue 6 years ago • 23 comments

Hi,

I know there are ALOT of posts related to this issue but I still didn't find a proper answer for this problem. I'm using this (GREAT ! ) module with nodes list ~10K . When trying to select a checkbox with a list of >1K - the system is lagging. In addition , using treeModel.expandAll() - is also lagging. Also programically using those lines are codes are showing poor preformance :

selectAllNodes() {
        const firstNode = this.tree.treeModel.getFirstRoot();
        firstNode.setIsSelected(true);
    }

    deselectAllNodes() {
        const firstNode = this.tree.treeModel.getFirstRoot();
        firstNode.setIsSelected(false);
    }

It is not clear if there is some kind of fix for those problems in the near future (or not..).

Plunker : https://plnkr.co/edit/yYRiJtGmpvG49JOvLw2m?p=info

mojo2405 avatar Apr 11 '18 09:04 mojo2405

I get terrible performance expanding nodes atm with 100's of nodes. I did a bit of digging and I think it might be related to all the events being fired.

If I inject a node with 130~ nested children and call expand() on each one to ensure they are expanded, my understanding is that each node then fires events causing a broadcast of 100's of events.

I don't need events at all, it would be nice if you could turn them off. In the meantime I will have to move to ng2-tree or similar.

ScottSpittle avatar Apr 12 '18 05:04 ScottSpittle

Hi,

  1. For large trees you should use virtual scroll. Please check out the docs: https://angular2-tree.readme.io/docs/large-trees

  2. I tried using it on the above plunkr and still got very bad performance, so this is something that needs to be investigated, because the local example I have works fast. Maybe it's because of deep nesting (my local example has 2 levels).

adamkleingit avatar Apr 12 '18 11:04 adamkleingit

But anyway doing expandAll on a huge tree will not be fast. I would consider either using lazy loading, or not allowing the user to do expand all.

adamkleingit avatar Apr 12 '18 11:04 adamkleingit

Also I see that if you click a checkbox in the top node then it's slow. Need to check if it's possible to make that process faster.

adamkleingit avatar Apr 12 '18 11:04 adamkleingit

Hey Adam, Thanks for the response. I hope you'll have the time to fix it soon.

mojo2405 avatar Apr 12 '18 12:04 mojo2405

I have also seen extremely poor performance with the tree expand/collapse functionality. We have a tree that is fairly large and in IE11 it performs very poor. It seems to lock up the browser for maybe ~15 seconds when doing expand / collapse on several nodes.

We are using the virtual scroll functionality which works great except for the high price of the expand/collapse. Chrome seems to perform much better (no surprise there), but IE is still in use by so many people...

+1 on any performance improvements that can be made!

This is an awesome tree control, so I'd hate to jump ship - but the performance is a pretty big problem right now for us...

mgpGit-zz avatar Apr 16 '18 16:04 mgpGit-zz

Hello I don't know if it helps but also with huge trees expandAll and collapseAll can be quite fast with some drawbacks which @adamkleingit may be able to find and explain.

  public setExpandedNodes(expandedNodeIds: any) {
    this.tree.treeModel.expandedNodeIds = expandedNodeIds;
    this.refresh();
  }

  public collapseAll() {
    let expandedNodeIds: any = {};
    this.setExpandedNodes(expandedNodeIds);
  }
  
  public expandAll() {
    let expandedNodeIds: any = {};
    for (let node of this.tree.treeModel.roots) {
      expandedNodeIds = this.updateNode(node, expandedNodeIds, true)
    }
    this.setExpandedNodes(expandedNodeIds);
  }

  private updateNode(node: TreeNode, expandedNodeIds: any, expand: boolean) {
    let newExp = expandedNodeIds
    let children = node.children
    if (children) {
      for (let child of children) {
        newExp = this.updateNode(child, newExp, expand);
      }
    }
    if (node.hasChildren) {
      return Object.assign({}, newExp, {[node.id]: expand});
    }
    return newExp;
  }

One problem is that no event is fired for the expand and collapse but this may not be a problem for you

MichaelJakubec avatar Apr 17 '18 13:04 MichaelJakubec

Thanks @amunra2112!

I'm blown away at the speed the above works! Why would anyone want events for the expand / collapse to fire when expanding/collapsing all or when programmatically expanding/collapsing a large set of nodes.

It seems that the "toggleExpanded" fires correctly when clicking the expand/collapse icon. So it only doesn't trigger the "toggleExpanded" when doing the actions above - perfect!

@adamkleingit - are there any downsides to using the above?

Thanks again! Mike

mgpGit-zz avatar Apr 17 '18 16:04 mgpGit-zz

@amunra2112 does this require changing the source code or can we override this in our own component.ts?

Is there a way to show an expand/collapse button when hovering on a branch node? This doesn't solve the problem totally, but for some it may alleviate some performance issues.

nshelms avatar Apr 21 '18 19:04 nshelms

@nshelms No you mustn't change anything in the original source code. These functions above can be written in your own component.ts

MichaelJakubec avatar Apr 22 '18 08:04 MichaelJakubec

@amunra2112 Thanks for that. Can you please take a look also in the checkbox moethods ? we have the same preformance issue there as well

mojo2405 avatar Apr 22 '18 08:04 mojo2405

@nshelms I had a look in the code but please be aware I'm not a contributor of this project, I'm just user of this lib as you are. So I can't tell you which unwanted side effects this has. And as long as Adam or any other official contributor with knowledge of the code tells something about this, my ideas can bring more harm then help.

So in the treeModel class there is the method/function

@action setSelectedNode(node, value) {
    this.selectedLeafNodeIds = Object.assign({}, this.selectedLeafNodeIds, {[node.id]: value});

    if (value) {
      node.focus();
      this.fireEvent({ eventName: TREE_EVENTS.select, node });
    } else {
      this.fireEvent({ eventName: TREE_EVENTS.deselect, node });
    }
}

So i assume that you may do the same as in my expand all method.

public selectAll() {
    let selectedLeafNodeIds: any = {};
    for (let node of this.tree.treeModel.roots) {
      selectedLeafNodeIds = this.updateSelectedNodes(node, selectedLeafNodeIds, true)
    }
    this.tree.treeModel.selectedLeafNodeIds = selectedLeafNodeIds;
    this.tree.sizeChanged();
  }

  private updateSelectedNodes(node: TreeNode, selectedLeafNodeIds: any, expand: boolean) {
    let newSel = selectedLeafNodeIds
    
    if (node.hasChildren()) {
      let children = node.children
      for (let child of children) {
        newExp = this.updateSelectedNodes(child, newSel, expand);
      }
    } else {
      return Object.assign({}, newSel, {[node.id]: expand});
    }
    return newExp;
  }

I haven't tested this code at all, so please try to run it yourself. I just had a look in the code and made some assumptions. Maybe this isn't working at all

BW

MichaelJakubec avatar Apr 22 '18 09:04 MichaelJakubec

@adamkleingit any thoughts ?

mojo2405 avatar May 03 '18 08:05 mojo2405

This works fast: this.tree.treeModel.doForAll((node: TreeNode) => node.isSelected && node.setIsSelected(false));

o-tkach avatar May 18 '18 13:05 o-tkach

@amunra2112 I've been going crazy with this problem, thanks for the help!

Vanguard90 avatar Dec 07 '18 12:12 Vanguard90

Sorry for the late response. As you must have noticed - I'm letting the amazing community handle these issues, and I make every person who contributes good code a maintainer of the lib. I think the above solution makes sense - expanding the tree programmatically usually doesn't require firing events. The immediate solution that comes to mind is to pass a flag called fireEvents which is true by default to be backwards-compatible. The question is - will it still be fast enough (it's just a boolean test so it should be fine I guess). Anyone wants to make a PR?

@amunra2112 @mpGitHub @mojo2405

adamkleingit avatar Dec 07 '18 17:12 adamkleingit

I had similar poor performance with hundreds of tree elements. After a bit of profiling, it did appear to spend a bit of time in one method. Setting the option: useTriState: false, helped a bunch in spite of the fact I don't use triStateCheckboxes. Is this variable not getting a proper default?

WilliamRClark avatar Apr 19 '19 15:04 WilliamRClark

@nshelms I had a look in the code but please be aware I'm not a contributor of this project, I'm just user of this lib as you are. So I can't tell you which unwanted side effects this has. And as long as Adam or any other official contributor with knowledge of the code tells something about this, my ideas can bring more harm then help.

So in the treeModel class there is the method/function

@action setSelectedNode(node, value) {
    this.selectedLeafNodeIds = Object.assign({}, this.selectedLeafNodeIds, {[node.id]: value});

    if (value) {
      node.focus();
      this.fireEvent({ eventName: TREE_EVENTS.select, node });
    } else {
      this.fireEvent({ eventName: TREE_EVENTS.deselect, node });
    }
}

So i assume that you may do the same as in my expand all method.

public selectAll() {
    let selectedLeafNodeIds: any = {};
    for (let node of this.tree.treeModel.roots) {
      selectedLeafNodeIds = this.updateSelectedNodes(node, selectedLeafNodeIds, true)
    }
    this.tree.treeModel.selectedLeafNodeIds = selectedLeafNodeIds;
    this.tree.sizeChanged();
  }

  private updateSelectedNodes(node: TreeNode, selectedLeafNodeIds: any, expand: boolean) {
    let newSel = selectedLeafNodeIds
    
    if (node.hasChildren()) {
      let children = node.children
      for (let child of children) {
        newExp = this.updateSelectedNodes(child, newSel, expand);
      }
    } else {
      return Object.assign({}, newSel, {[node.id]: expand});
    }
    return newExp;
  }

I haven't tested this code at all, so please try to run it yourself. I just had a look in the code and made some assumptions. Maybe this isn't working at all

BW

Needs a little adjasments but its helped.

michaelnem avatar Apr 02 '20 15:04 michaelnem

I know this thread is old but I run into same issue. Here's snippet for checkbox performance issue (Select all) that you can pass with options I managed to reduce delay from 5-10 sec to ~160ms (I have ~2.5k elements in tree).

const actionMapping: IActionMapping = {
    mouse: {
        checkboxClick: (tree: TreeModel, node: TreeNode, $event: any) => {
            if (!node) {
                return;
            }
            const value = !node.isSelected;
            const selectedLeadNodes = tree.selectedLeafNodeIds;
            setNodeSelected(tree, node, value);
            tree.selectedLeafNodeIds = Object.assign({}, selectedLeadNodes, {[node.id]: value});

            function setNodeSelected(tree, node, value) {
                if (node.isSelectable()) {
                    selectedLeadNodes[node.id] = value;
                } else {
                    node.visibleChildren.forEach((child) => setNodeSelected(tree, child, value));
                }
            }
        }
    }
};

const options: ITreeOptions = {
    useCheckbox: true,
    useVirtualScroll: true,
    nodeHeight: 25,
    actionMapping
};

mfig avatar Aug 13 '21 23:08 mfig

I have just come across this issue. I have a tree with roughly 1000 nodes 2-4 levels deep. The snippet @mfig posted helped when selecting the parent node of those items the first time (Takes about 300-400ms). However when I then deselect it and all children it takes about 15sec and about as long when I then select the node again. Any ideas that might help?

Mackemania avatar Oct 07 '21 13:10 Mackemania

Hello, @adamkleingit

  • Same here, the browser goes down(sending wait or kill popup message), very lower performance when expanding all tree-nodes with multi-level and large tree-nodes.

I have added async-await when expanding all nodes. It's taking a lot of time. But in a way, it is good that the browser not hanging. like,

expandAll() {
    this.loaderService.show();
    setTimeout(async () => {
      await this.tree.treeModel.expandAll();
      this.loaderService.hide();
    }, 1000);
  }

but, I haven't a solution for selectAll. When selecting a parent node, which has multiple child with 2-3 levels. I can't add async-await because select event emitting for every last single child node(500 times, 1000 times). That time browser goes down and displays a wait or kill popup.

have any solutions? need help pls.

nikunjgadhiya-rishabh avatar Dec 03 '21 11:12 nikunjgadhiya-rishabh

Hi All,

I am facing the same performance issue (as mentioned by @Mackemania ) with checkbox selection/de-selection on parent node having large number of children (~10,000). I have virtual scrolling enabled. Just wanted to know if anyone has found a good workaround for this issue or any direction where I can look into?

EDIT: It's so bad that the page crashes.

Thanks.

madman-maverick avatar Sep 09 '22 13:09 madman-maverick

I was facing this same performance issue while expanding nodes. For large data sets this method would result in crashing of the chrome page. One way I was able to bypass the problem was like this:

I used Virtual scroll as suggested:

public options: ITreeOptions = {
        idField: 'id',
        useTriState: false,
        useVirtualScroll: true,
        nodeHeight: 25,
    }; 

Also, I made the function call Async to prevent browser from crashing. Also, Once all the nodes have been called, I update the tree Mode and redraw the tree.

    private async refreshNodesMenuOptions(): Promise<void> {
        const iterationPromises = this.productNodes.map(node => this.treeNodeService.iterateNodesToRefresh(node, this.tree));
        await Promise.all(iterationPromises);
        await Promise.all([
            this.tree?.treeModel.update(),
            this.tree?.sizeChanged()
        ]);
    }

Inside my function, instead of calling node.expand(), I have simply marked the node as true. node.expand() is a bottleneck in performance. And since I am updating and redrawing the tree later, the nodes get expanded.

 const node = tree.treeModel.getNodeById(id);
                if (node) {
                    tree.treeModel.expandedNodeIds[ node.id ] = true;
                }

This was able to solve my problem the performance issue to some extent.

purushpsm147 avatar Sep 01 '23 06:09 purushpsm147