Opened 12 years ago
Last modified 4 years ago
#9411 assigned enhancement
Remove dojo.indexOf() call in dojo.NodeList-traverse's _getUniqueAsNodeList
Reported by: | Les | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | 1.15 |
Component: | Query | Version: | 1.3.0 |
Keywords: | Cc: | Kitson Kelly | |
Blocked By: | Blocking: |
Description (last modified by )
The for loop in the code fragment below could be further optimized. According to this article loop #3 is faster compared to loop #2. There are other places in dojo where loop #3 could be used instead of loop #2.
_getUniqueAsNodeList: function(nodes){ // summary: // given a list of nodes, make sure only unique // elements are returned as our NodeList object. // Does not call _stash(). var ary = []; //Using for loop for better speed. for(var i = 0, node; node = nodes[i]; i++){ //Should be a faster way to do this. dojo.query has a private //_zip function that may be inspirational, but there are pathways //in query that force nozip? if(node.nodeType == 1 && dojo.indexOf(ary, node) == -1){ ary.push(node); } } return dojo._NodeListCtor._wrap(ary); //dojo.NodeList },
Change History (9)
comment:1 Changed 12 years ago by
Milestone: | tbd → 1.4 |
---|---|
Owner: | changed from anonymous to James Burke |
comment:2 Changed 12 years ago by
FYI we tested loop performance (well in particular live collections) in the dojo community and found the best syntax to be:
while (el = els[i++]) { }
comment:3 Changed 11 years ago by
Milestone: | 1.4 → 1.5 |
---|---|
Summary: | JavaScript loop performance → Remove dojo.indexOf() call in dojo.NodeList-traverse's _getUniqueAsNodeList |
Changing summary to indicate the larger issue of indexOf. To solve this we need to pull out the node ID stuff used in dojo.query to be reusable by other modules, but that is a larger task than what I have time for now.
comment:4 Changed 11 years ago by
Milestone: | 1.5 → 1.6 |
---|
comment:5 Changed 10 years ago by
Milestone: | 1.6 → future |
---|
(sadly) punting seemingly abandoned ticket and meta tickets to future
comment:6 Changed 9 years ago by
Component: | General → Query |
---|---|
Owner: | changed from James Burke to Kris Zyp |
Status: | new → assigned |
indexOf() call is still there.
comment:7 Changed 9 years ago by
Cc: | Kitson Kelly added |
---|---|
Description: | modified (diff) |
BTW, kitsonk's parser patch in #14591 does the same thing as acme, adding temporary id's to nodes if they don't have a browser generated uniqueId/GUID.
comment:8 Changed 5 years ago by
Milestone: | future → 1.12 |
---|---|
Owner: | Kris Zyp deleted |
comment:9 Changed 4 years ago by
Milestone: | 1.13 → 1.15 |
---|
Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.
The loop tests at the link above are a bit deceptive: they only test an empty loop. The first thing I will want to do in the loop is to access node[i] (actually do two calls for it) so assigning in the for statement made more sense. I expect the loop tests at that URL to have different results if inside each loop the node[i] was then grabbed.
But this function is horribly inefficient for a larger reason, the dojo.indexOf call which does another loop. So the larger fix is to use some of the dojo.query machinery to give nodes a unique ID property and use that to help things along.