Opened 10 years ago

Last modified 3 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 bill)

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 10 years ago by James Burke

Milestone: tbd1.4
Owner: changed from anonymous to James Burke

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.

comment:2 Changed 10 years ago by bill

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++]) { }

See source:trunk/bench/live_collections.html.

comment:3 Changed 10 years ago by James Burke

Milestone: 1.41.5
Summary: JavaScript loop performanceRemove 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 9 years ago by Adam Peller

Milestone: 1.51.6

comment:5 Changed 8 years ago by bill

Milestone: 1.6future

(sadly) punting seemingly abandoned ticket and meta tickets to future

comment:6 Changed 7 years ago by bill

Component: GeneralQuery
Owner: changed from James Burke to Kris Zyp
Status: newassigned

indexOf() call is still there.

comment:7 Changed 7 years ago by bill

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 3 years ago by dylan

Milestone: future1.12
Owner: Kris Zyp deleted

comment:9 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.