CirclonGroup / angular-tree-component

A simple yet powerful tree component for Angular (>=2)
https://angular2-tree.readme.io/docs
MIT License
1.1k stars 494 forks source link

Slow preformance on expand and select checkboxes #604

Open mojo2405 opened 6 years ago

mojo2405 commented 6 years ago

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

ScottSpittle commented 6 years ago

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.

adamkleingit commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

mojo2405 commented 6 years ago

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

mgpGit-zz commented 6 years ago

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...

MichaelJakubec commented 6 years ago

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

mgpGit-zz commented 6 years ago

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

nshelms commented 6 years ago

@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.

MichaelJakubec commented 6 years ago

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

mojo2405 commented 6 years ago

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

MichaelJakubec commented 6 years ago

@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

mojo2405 commented 6 years ago

@adamkleingit any thoughts ?

o-tkach commented 6 years ago

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

Vanguard90 commented 5 years ago

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

adamkleingit commented 5 years ago

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

WilliamRClark commented 5 years ago

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?

michaelnem commented 4 years ago

@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.

mfig commented 3 years ago

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
};
Mackemania commented 3 years ago

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?

nikunjgadhiya-rishabh commented 2 years ago

Hello, @adamkleingit

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.

madman-maverick commented 2 years ago

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.

purushpsm147 commented 1 year ago

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.