#9994 closed task (wontfix)
dijit._Container refactoring
Reported by: | Les | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | 2.0 |
Component: | Dijit | Version: | 1.3.2 |
Keywords: | dijit._Container | Cc: | |
Blocked By: | Blocking: |
Description
I created the new queryChildren() function and used it (when possible) where the getChildren() function was used before. In many cases we don't need to have the widgets, but just the widget DOM nodes. I see a significant performance improvement.
Test:
Go here: http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Tree.html
Run:
dijit._Widget.prototype.queryChildren = function(){ return dojo.query("> [widgetId]", this.containerNode); }; dijit._Widget.prototype.addChildNew = function(/*Widget*/ widget, /*int?*/ insertIndex){ // summary: // Makes the given widget a child of this widget. // description: // Inserts specified child widget's dom node as a child of this widget's // container node, and possibly does other processing (such as layout). var refNode = this.containerNode; if(insertIndex && typeof insertIndex == "number"){ var children = this.queryChildren(); if(children && children.length >= insertIndex){ refNode = children[insertIndex-1]; insertIndex = "after"; } } dojo.place(widget.domNode, refNode, insertIndex); // If I've been started but the child widget hasn't been started, // start it now. Make sure to do this after widget has been // inserted into the DOM tree, so it can see that it's being controlled by me, // so it doesn't try to size itself. if(this._started && !widget._started){ widget.startup(); } return widget; // Widget }; dijit._Widget.prototype.removeChildNew = function(/*Widget or int*/ widget){ // summary: // Removes the passed widget instance from this widget but does // not destroy it. You can also pass in an integer indicating // the index within the container to remove var node; if(typeof widget == "number" && widget > 0){ node = this.queryChildren()[widget]; widget = dijit.byNode(node); } else if(widget && widget.domNode){ node = widget.domNode; } if(node){ node.parentNode.removeChild(node); } // detach but don't destroy return widget; // Widget }; dijit._Widget.prototype.addChild = function(/*Widget*/ widget, /*int?*/ insertIndex){ // summary: // Makes the given widget a child of this widget. // description: // Inserts specified child widget's dom node as a child of this widget's // container node, and possibly does other processing (such as layout). var refNode = this.containerNode; if(insertIndex && typeof insertIndex == "number"){ var children = this.queryChildren(); if(children && children.length >= insertIndex){ refNode = children[insertIndex-1]; insertIndex = "after"; } } dojo.place(widget.domNode, refNode, insertIndex); // If I've been started but the child widget hasn't been started, // start it now. Make sure to do this after widget has been // inserted into the DOM tree, so it can see that it's being controlled by me, // so it doesn't try to size itself. if(this._started && !widget._started){ widget.startup(); } return widget; // Widget }; dijit._Widget.prototype.getChildrenNew = function(){ // summary: // Returns array of children widgets. // description: // Returns the widgets that are directly under this.containerNode. return this.queryChildren().map(dijit.byNode); // Widget[] }; dijit._Widget.prototype.hasChildrenNew = function(){ // summary: // Returns true if widget has children, i.e. if this.containerNode contains something. return this.queryChildren().length > 0; // Boolean }; dijit._Widget.prototype.getIndexOfChildNew = function(/*Widget*/ child){ // summary: // Gets the index of the child in this container or -1 if not found return dojo.indexOf(this.queryChildren(), child.domNode); // int }; var parent = dijit.byId('dijit__TreeNode_0'); var child = dijit.byId('dijit__TreeNode_6'); // Europe, index 3 console.time('getChildren'); for(var i=0; i<1000; i++) { parent.getChildren(); } console.timeEnd('getChildren'); console.time('queryChildren'); for(var i=0; i<1000; i++) { parent.queryChildren(); } console.timeEnd('queryChildren'); dijit._Widget.prototype.getIndexOfChildNew = function(/*Widget*/ child){ // summary: // Gets the index of the child in this container or -1 if not found return dojo.indexOf(this.queryChildren(), child.domNode); // int } // getIndexOfChild test console.time('getIndexOfChild'); for(var i=0; i<1000; i++) { parent.getIndexOfChild(child); } console.timeEnd('getIndexOfChild'); console.time('getIndexOfChildNew'); for(var i=0; i<1000; i++) { parent.getIndexOfChildNew(child); } console.timeEnd('getIndexOfChildNew'); // removeChild/addChild by index test console.time('removeAdd'); for(var i=0; i<1000; i++) { parent.removeChild(3); parent.addChild(child, 3); } console.timeEnd('removeAdd'); console.time('removeAddNew'); for(var i=0; i<1000; i++) { var widget = parent.removeChildNew(3); parent.addChildNew(widget, 3); } console.timeEnd('removeAddNew'); console.time('getIndexOfChild'); for(var i=0; i<1000; i++) { parent.getIndexOfChild(child); } console.timeEnd('getIndexOfChild'); console.time('getIndexOfChildNew'); for(var i=0; i<1000; i++) { parent.getIndexOfChildNew(child); } console.timeEnd('getIndexOfChildNew'); console.log('getChildren: ' + (parent.getChildrenNew().length == parent.getChildren().length)); console.log('hasChildren: ' + (parent.hasChildrenNew() == parent.hasChildren())); console.log('getIndexOfChild: ' + (parent.getIndexOfChildNew(child) == parent.getIndexOfChild(child)));
Results:
FF 3.5 getChildren --> queryChildren: 233ms --> 144ms getIndexOfChild -- getIndexOfChildNew: 254ms --> 155ms removeAdd --> removeAddNew: 1237ms --> 567ms getIndexOfChild --> getIndexOfChildNew: 254ms --> 153ms
IE6 getChildren --> queryChildren: 1594ms --> 1000ms getIndexOfChild -- getIndexOfChildNew: 1594ms --> 1063ms removeAdd --> removeAddNew: 7578ms --> 3063ms getIndexOfChild --> getIndexOfChildNew: 1594ms --> 1063ms
Chrome 4 getChildren --> queryChildren: 184ms --> 112ms getIndexOfChild -- getIndexOfChildNew: 184ms --> 111ms removeAdd --> removeAddNew: 876ms --> 390ms getIndexOfChild --> getIndexOfChildNew: 180ms --> 112ms
Change History (9)
comment:1 Changed 11 years ago by
Milestone: | tbd → 2.0 |
---|
comment:2 Changed 11 years ago by
Also, these methods should all be on _Container, right, not _Widget? getChildren() for _Widget is more general since it can recurse to any level until it finds a widget (whereas _Container is structured, everything is directly under this.containerNode).
These methods should all be on _Container. I did it just for testing.
I'm not familiar with the AccordionContainer?, but all these changes should be generic. All modified methods should behave exactly the same as the existing methods. There will be no change at all in functionality of all container methods.
The point is that we don't need the expensive mapping from nodes to widgets that getChildren() does. We often need just the widget nodes, so the mapping step can be skipped.
You should be able to safely add these changes for 1.4
comment:3 Changed 11 years ago by
Try it out, I think you'll see that it doesn't work. Try adding and removing panes from an AccordionContainer.
comment:4 Changed 11 years ago by
Please help me. Write a small example that works using the existing Container methods, and I'll update it using the new methods that I modified.
Use this page if you can: http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/layout/test_AccordionContainer.html
The dijit._Container.queryChildren() method that I added can be private. It's just a helper, but I made it public in case someone wanted to use it. The container changes are really quite simple. Actually, they simplify the exisiting code. I'm confident they will work.
comment:5 Changed 11 years ago by
We already have such examples, see dijit/tests/layout/AccordionContainer.html. It's an automated test (look in your console output), and you can also do operations manually.
Many widgets subclass _Container. _Container's addChild(), removeChild(), destroyRecursive() etc. methods are all setup to work based on the subclass's definition of getChildren().
comment:6 Changed 11 years ago by
Many widgets subclass _Container. _Container's addChild(), removeChild(), destroyRecursive() etc. methods are all setup to work based on the subclass's definition of getChildren().
Right, and the way I set up the example, I overwrote those overwritten methods, but it works for the Tree. Anyway, I believe the original changes are correct and effective.
I can create a regular (formal) patch that would be easier to test. This is just a performance enhancement. I'm sure you are busy fixing bugs for 1.4, and this is something that we can return to for a future release.
comment:7 Changed 11 years ago by
The problem is though that, for example, on AccordionContainer?, getChildren() only returns half of the widgets (hiding the AccordionButton? widgets), but addChild(), removeChild(), etc. still work even though AccordionContainer? doesn't redefine those methods to deal w/that fact.
Now I have doubts. I don't think it's possible to always use queryChildren() in place of getChildren(). It may not work when getChildren() is overwritten b/c getChildren() may return fewer widgets, i.e queryChildren() will return more elements in some cases.
Feel free to close this ticket. It's not valid.
comment:8 Changed 11 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
OK, closing in favor of #10087.
comment:9 Changed 11 years ago by
Type: | enhancement → task |
---|
seems like these aren't really enhancements from the users' point of view, labeling as tasks instead
Interesting. Looks like it definitely will speed things up.
The problem is though that, for example, on AccordionContainer, getChildren() only returns half of the widgets (hiding the AccordionButton widgets), but addChild(), removeChild(), etc. still work even though AccordionContainer doesn't redefine those methods to deal w/that fact.
I think if every widget has a queryChildren() method that returned the DOM nodes of the official children, that we could do this change, but we can't make that change until 2.0. Do you see a different way?
Also, these methods should all be on _Container, right, not _Widget? getChildren() for _Widget is more general since it can recurse to any level until it finds a widget (whereas _Container is structured, everything is directly under this.containerNode).