Looks like we can't reproduce the issue or there's a problem in the test case provided.
  1. #1
    Ext JS Premium Member westy's Avatar
    Join Date
    Feb 2009
    Location
    Bath, UK
    Posts
    966
    Vote Rating
    72
    westy is just really nice westy is just really nice westy is just really nice westy is just really nice westy is just really nice

      0  

    Default [4.1.0] NodeInterface.removeAll - performance increase

    [4.1.0] NodeInterface.removeAll - performance increase


    Hi,

    Been looking into why, in IE7, refreshing a tree node can take over 30 seconds with around 200 child nodes.
    This causes the infamous, 'this script is taking too long, do you want to stop it' message (or words to that effect) four or fives times!

    It appears that the main culprit is NodeInterface.removeAll; it's simplistic approach is to call remove in a loop.
    The remove method does a lot of processing that is not needed in the removeAll case, since we know we are removing ALL children.

    Anyway, the following reduces the time spent from around 30 seconds (on my i7 desktop with 12Gb and a fast SSD) to less than a second.
    I'm not sure it's perfect, but it seems to work okay for me.

    Code:
    /**
     * Removes all child nodes from this node.
     * This is my override to speed up the processing but a huge amount... previous version simply calls remove,
     * and does a lot of pointless housekeeping, since in this case we know we are removing ALL nodes.
     * @param {Boolean} [destroy=false] True to destroy the node upon removal.
     * @return {Ext.data.NodeInterface} this
     */
    removeAll : function(destroy, suppressEvents) {
        var me = this,
            childNodes = me.childNodes,
            node,
            index,
            haveRemoveListeners = me.hasListeners.remove;
    
    
        while ((node = childNodes[childNodes.length - 1])) {
            index = childNodes.length - 1;
    
    
            // Remove it from childNodes collection
            delete childNodes[index];
            childNodes.length = index;
    
    
            // Inform any listeners
            if ((suppressEvents !== true) && haveRemoveListeners) {
                me.fireEvent("remove", me, node, false);
            }
    
    
            if (destroy) {
                node.destroy(true);
            } else {
                node.clear();
            }
        }
    
    
        // This node will no longer have any child nodes, so update ourselves.
        me.set('loaded', false);
    
    
        return me;
    }
    Now, I'm not 100% sure it's doing everything needed, but seems to work okay for me (and I have a lot of complex trees). Ideally I'd like one event at the end, rather than one for each node, but think it's needed with the current set up...

    I considered setting 'expanded' to false at the end as well, since when removeAll is called in isolation this puts the tree back into a nice state, but this seems to break the load process, where removeAll is called if clearOnLoad is set.

    Since NodeInterface is a complete pain to override I'll include that too since others may want to do some testing with it too!
    Also includes my fix for destroy, and you can see the original remove method, that I used as a basis for the innards of removeAll.
    Code:
    Ext.require('Ext.data.NodeInterface',
        function() {
            Ext.override(Ext.data.NodeInterface, {
                statics: {
                    /**
                     * Fix to reset childNodes to [] rather than null.
                     * See: http://www.sencha.com/forum/showthread.php?203853-TreeStore-bugs-appendChild()-load()
                     */
                    decorate: function(modelClass) {
                        this.callParent(arguments);
    
    
                        // Replacing implementation of destroy, supplied from getPrototypeBody.
                        modelClass.override({
                            /**
                             * Destroys the node.
                             */
                            destroy : function(silent) {
                                /*
                                 * Silent is to be used in a number of cases
                                 * 1) When setRoot is called.
                                 * 2) When destroy on the tree is called
                                 * 3) For destroying child nodes on a node
                                 */
                                var me      = this,
                                    options = me.destroyOptions,
                                    nodes   = me.childNodes,
                                    nLen    = nodes.length,
                                    n;
    
    
                                if (silent === true) {
                                    me.clear(true);
    
    
                                    for (n = 0; n < nLen; n++) {
                                        nodes[n].destroy(true);
                                    }
    
    
                                    me.childNodes = [];         // <WestyFix> This was getting set to null.
                                    delete me.destroyOptions;
                                    me.callOverridden([options]);
                                } else {
                                    me.destroyOptions = silent;
                                    // overridden method will be called, since remove will end up calling destroy(true);
                                    me.remove(true);
                                }
                            },
    
    
                            /**
                             * Removes all child nodes from this node.
                             * This is my override to speed up the processing but a huge amount... previous version simply calls remove,
                             * and does a lot of pointless housekeeping, since in this case we know we are removing ALL nodes.
                             * @param {Boolean} [destroy=false] True to destroy the node upon removal.
                             * @return {Ext.data.NodeInterface} this
                             */
                            removeAll : function(destroy, suppressEvents) {
                                var me = this,
                                    childNodes = me.childNodes,
                                    node,
                                    index,
                                    haveRemoveListeners = me.hasListeners.remove;
    
    
                                while ((node = childNodes[childNodes.length - 1])) {
                                    index = childNodes.length - 1;
    
    
                                    // Remove it from childNodes collection
                                    delete childNodes[index];
                                    childNodes.length = index;
    
    
                                    // Inform any listeners
                                    if ((suppressEvents !== true) && haveRemoveListeners) {
                                        me.fireEvent("remove", me, node, false);
                                    }
    
    
                                    if (destroy) {
                                        node.destroy(true);
                                    } else {
                                        node.clear();
                                    }
                                }
    
    
                                // This node will no longer have any child nodes, so update ourselves.
                                me.set('loaded', false);
    
    
                                return me;
                            }
    
    
                            // removeChild : function(node, destroy, suppressEvents, isMove) {
                            //     var me = this,
                            //         index = me.indexOf(node),
                            //         i, childCount;
    
    
                            //     if (index == -1 || (suppressEvents !== true && (!me.hasListeners.beforeremove || me.fireEvent("beforeremove", me, node, !!isMove) === false))) {
                            //         return false;
                            //     }
    
    
                            //     // remove it from childNodes collection
                            //     Ext.Array.erase(me.childNodes, index, 1);
    
    
                            //     // update child refs
                            //     if (me.firstChild == node) {
                            //         me.setFirstChild(node.nextSibling);
                            //     }
                            //     if (me.lastChild == node) {
                            //         me.setLastChild(node.previousSibling);
                            //     }
    
    
                            //     // update siblings
                            //     if (node.previousSibling) {
                            //         node.previousSibling.nextSibling = node.nextSibling;
                            //     }
                            //     if (node.nextSibling) {
                            //         node.nextSibling.previousSibling = node.previousSibling;
                            //     }
    
    
                            //     // update the info for all siblings starting at the index before the node's old index (or 0 if the removed node was the firstChild)
                            //     for(i = index > 0 ? index - 1 : 0, childCount = me.childNodes.length; i < childCount; i++) {
                            //         me.childNodes[i].updateInfo();
                            //     }
    
    
                            //     // If this node suddenly doesnt have childnodes anymore, update myself
                            //     if (!me.childNodes.length) {
                            //         me.set('loaded', me.isLoaded());
                            //     }
    
    
                            //     if (suppressEvents !== true) {
                            //         if (me.hasListeners.remove) {
                            //             me.fireEvent("remove", me, node, !!isMove);
                            //         }
                            //     }
    
    
                            //     if (destroy) {
                            //         node.destroy(true);
                            //     } else {
                            //         node.clear();
                            //     }
    
    
                            //     return node;
                            // }
                        });
                    }
                }
            });
        }
    );
    Hope the author of NodeInterface can be suitably punished for this horrible horrible 'class'

    Oh, and apologies for not using the bug reporting form (again), but I don't think it really applies in this case...

    Regards,
    Westy

    PS: Please, post any thoughts or improvements here.
    I'm now off to try and speed up node creation, will report back if find any magic bullets
    Product Architect
    Altus Ltd.

  2. #2
    Ext JS Premium Member westy's Avatar
    Join Date
    Feb 2009
    Location
    Bath, UK
    Posts
    966
    Vote Rating
    72
    westy is just really nice westy is just really nice westy is just really nice westy is just really nice westy is just really nice

      0  

    Default


    Looks like in 4.1.1 should use:
    Code:
    me.triggerUIUpdate();
    Instead of:
    Code:
    me.set('loaded', false);
    Not tried it, since 4.1.1 has issues with it that prevent me from updating to it.
    Product Architect
    Altus Ltd.

  3. #3
    Sencha - Ext JS Dev Team evant's Avatar
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    17,167
    Vote Rating
    674
    evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute

      0  

    Default


    There have been some improvements made already similar to what you've suggested, hopefully should make it into the next patch release.
    Evan Trimboli
    Sencha Developer
    Twitter - @evantrimboli
    Don't be afraid of the source code!

  4. #4
    Ext JS Premium Member westy's Avatar
    Join Date
    Feb 2009
    Location
    Bath, UK
    Posts
    966
    Vote Rating
    72
    westy is just really nice westy is just really nice westy is just really nice westy is just really nice westy is just really nice

      0  

    Default


    Quote Originally Posted by evant View Post
    There have been some improvements made already similar to what you've suggested, hopefully should make it into the next patch release.
    Excellent, look forward to testing them!
    Product Architect
    Altus Ltd.

Thread Participants: 1

Tags for this Thread