Opened 10 years ago

Closed 10 years ago

#9915 closed task (fixed)

WidgetSet performance issue

Reported by: Les Owned by: bill
Priority: high Milestone: 1.5
Component: Dijit Version: 1.3.2
Keywords: WidgetSet Cc:
Blocked By: Blocking:

Description

The code below is quite inefficient:

for(id in this._hash){
	func.call(thisObj || dojo.global, this._hash[id], i++, this._hash);
}

This is because dojo.global is used if thisObj is not provided, and accessing global variables is slow. I'd suggest modifying this code like this:

var obj = thisObj || dojo.global;
for(id in this._hash){
	func.call(obj, this._hash[id], i++, this._hash);
}

This impacts forEach(), filter(), every() and some().

Test setup

Go to: http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Menu.html#

Run:

dojo.extend(dijit.WidgetSet, {
	filterNew: function(/*Function*/ filter, /* Object? */thisObj){
		// summary:
		//		Filter down this WidgetSet to a smaller new WidgetSet
		//		Works the same as `dojo.filter` and `dojo.NodeList.filter`
		//		
		// filter:
		//		Callback function to test truthiness. Is passed the widget
		//		reference and the pseudo-index in the object. 
		//
		// thisObj: Object?
		//		Option scope to use for the filter function. 
		//
		// example:
		//		Arbitrary: select the odd widgets in this list
		//		|	dijit.registry.filter(function(w, i){
		//		|		return i % 2 == 0;
		//		|	}).forEach(function(w){ /* odd ones */ });

		var res = new dijit.WidgetSet(), i = 0, id, obj = thisObj || dojo.global;
		for(id in this._hash){
			var w = this._hash[id];
			if(filter.call(obj, w, i++, this._hash)){
				res.add(w);
			}
		}
		return res; // dijit.WidgetSet
	}
});

console.time('old');
for(var i=0; i<1000; i++) {
dijit.registry.filter(function(w, i){ return i % 2 == 0; });
}
console.timeEnd('old');

console.time('new');
for(var i=0; i<1000; i++) {
dijit.registry.filterNew(function(w, i){ return i % 2 == 0; });
}
console.timeEnd('new');

Results:

FF 3.5: 348ms --> 211ms
IE 6: 2454ms --> 1906ms

Change History (27)

comment:1 Changed 10 years ago by Les

Results on my home computer:

IE8: 1265ms --> 860ms
Safari 4: 126ms --> 118ms
Chrome 4: 143ms --> 120ms

comment:2 Changed 10 years ago by bill

Milestone: tbd1.4
Owner: set to bill
Status: newassigned

Wow big difference, OK thanks again for the fix, I'll check it in.

comment:3 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [20142]) Optimization for when thisObj is null, to access dojo.global outside the loop instead of inside. Fixes #9915 !strict. Thanks Les!

comment:4 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

I'm going to reopen this ticket since more could be done to improve performance by caching global objects in local variables.

Here's a test case:
http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Menu.html

Run:

(function(){
var _dijit = dijit;
dijit.byIdNew = function(/*String|Widget*/id){
	// summary:
	//		Returns a widget by it's id, or if passed a widget, no-op (like dojo.byId())
	return typeof id == "string" ? _dijit.registry._hash[id] : id; // Widget
};
})();

console.time('old');
for(var i=0; i<100000; i++) {
   dijit.byId('copy');
}
console.timeEnd('old');

console.time('new');
for(var i=0; i<100000; i++) {
   dijit.byIdNew('copy');
}
console.timeEnd('new');

Results:

FF 3.5: 326ms --> 231ms
IE6: 1234ms --> 969ms
IE8: 656ms --> 469ms (different PC)
Chrome 4 and Safari 4 show little improvement, but these are fast browsers.

I'm proposing to add a similar closure covering functions from dijit.getUniqueId() to dijit.getLastInTabbingOrder().

comment:5 Changed 10 years ago by Les

IE7: 1002ms --> 815ms (another PC)

comment:6 Changed 10 years ago by nic

Maybe we can use the same symbol in the closure, to help the code compression:
;(function(dijit){

dijit.byIdNew = ...

})(dijit);

comment:7 Changed 10 years ago by Les

I like Nic's suggestion, but let me improve :)

;(function(dojo, dijit){

   var widgetTypeCtr = {}; // Now local var.
    
   dijit.byIdNew = ...

})(dojo, dijit);

Now getUniqueId() still inside the closure can be simplified.

dijit.getUniqueId = function(/*String*/widgetType){
	// summary: Generates a unique id for a given widgetType

	var id;
	do{
		id = widgetType + "_" +
			(widgetType in widgetTypeCtr ?
				++widgetTypeCtr[widgetType] : widgetTypeCtr[widgetType] = 0);
	}while(dijit.byId(id));
	return id; // String
};

comment:8 Changed 10 years ago by nic

Hmm, maybe something like this is a bit faster in toArray:
var ar = [], i = 0, _hash = this._hash;

for(ar[i++] in _hash){}

return ar;

comment:9 in reply to:  8 Changed 10 years ago by nic

Replying to nic:

Hmm, maybe something like this is a bit faster in toArray:
var ar = [], i = 0, _hash = this._hash;

for(ar[i++] in _hash){}

return ar;

never mind, I pushed the keys into ar...

comment:10 Changed 10 years ago by Les

It is really interesting how the closure cleverly shadows global dojo and dijit objects with locals. Now any dojo or dijit code will run faster inside the closure.

See this test:

http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Menu.html
Run:

;(function(dojo, dijit){    
    console.time('inside');
    for(var i=0; i<100000; i++) {
       dojo.byId('formattingTable');
    }
    console.timeEnd('inside');
})(dojo, dijit);

console.time('outside');
for(var i=0; i<100000; i++) {
   dojo.byId('formattingTable');
}
console.timeEnd('outside');

Results: outside --> inside

FF 3.5:  417ms --> 163ms
IE8: 3219ms --> 2953ms

comment:11 Changed 10 years ago by bill

Core does the global --> local shadowing in a misguided attempt to save bytes; it replaces "dojo" with "d", which is less bytes before gzip but more bytes after. I didn't expect a big performance gain you mentioned though.

Speaking of performance, of dojo.byId(), wouldn't it be better to put dijit.registry._hash in a local variable?

comment:12 Changed 10 years ago by Les

...wouldn't it be better to put dijit.registry._hash in a local variable?

I'm not sure. I'd need to see some code.

What about this code? It will speed up any widget.

;(function(dojo, dijit){    	
	dojo.declare("MyWidget", dijit._Widget, {
		constructor: function(){	

		},
		
		test: function(){
			console.time('inside');
			for(var i=0; i<100000; i++) {
			   dojo.byId('nonexistant');
			}
			console.timeEnd('inside');
		}
	});	
})(dojo, dijit);

console.time('outside');
for(var i=0; i<100000; i++) {
   dojo.byId('nonexistant');
}
console.timeEnd('outside');

var widget = new MyWidget();
widget.test();

Results: outside --> inside

FF 3.5: 512ms --> 213ms

comment:13 Changed 10 years ago by Les

I didn't expect a big performance gain you mentioned though.

Not all browsers have shown a lot of performance gain on the last example, but none lost performance. FF has shown the most gain.

IE6: 49843ms --> 49281ms
IE7: 44970ms --> 49281ms
IE8: 49843ms --> 49281ms 
FF 3.0: 514ms --> 357ms
FF 3.5: 388ms --> 149ms (another test on a different PC)
Chrome 4: 296ms --> 274ms (Loop increased to 1000000) 
Safari 4 (Win): 766ms --> 558ms (Loop again increased to 1000000)

comment:14 Changed 10 years ago by Les

I modified the test just to have something different:

;(function(dojo, dijit){    
    console.time('inside');
    for(var i=0; i<100000; i++) {
       dijit.byId('copy');
    }
    console.timeEnd('inside');
})(dojo, dijit);

console.time('outside');
for(var i=0; i<100000; i++) {
   dijit.byId('copy');
}
console.timeEnd('outside');

Results:

IE6: 1250ms --> 891ms
IE7: 968ms --> 704ms
IE8: 734ms --> 500ms
FF 3.5: 315ms --> 134ms

This time IE has shown more improvement.

BTW Steve Souders wrote a few paragraphs in his book 'Even Faster Web Sites' about the use of local vs global variables in JavaScript?.

comment:15 Changed 10 years ago by Les

Another test:

;(function(dojo, dijit){    	
	dojo.declare("MyWidget", dijit._Widget, {
		constructor: function(){	

		},
		
		test: function(){
			console.time('inside');
			for(var i=0; i<100000; i++) {
			   dijit.byId('copy');
			}
			console.timeEnd('inside');
		}
	});	
})(dojo, dijit);

console.time('outside');
for(var i=0; i<100000; i++) {
   dijit.byId('copy');
}
console.timeEnd('outside');

var widget = new MyWidget();
widget.test();

Results:

IE6: 1219ms --> 922ms
FF 3.5: 321ms --> 143ms

Anyway, I believe that closures can speed up both dojo and dijit as shown in above examples.

comment:16 Changed 10 years ago by Les

Core does the global --> local shadowing in a misguided attempt to save bytes; it replaces "dojo" with "d", which is less bytes before gzip but more bytes after.

Also, the way global to local shadowing is done is not efficient or consistent.
See this code fragment from xhr.js.

(function(){
	var _d = dojo;
	var cfg = dojo.config;  // Here we are acescsing global dojo.  Why not to use _d.config? (_d.config is faster to access.)

	...
	
	// Here again we are acessing global dojo.  Why not to use _d.fieldToObject?  
	dojo.fieldToObject = function(/*DOMNode||String*/ inputNode){
	
	....

       // Here we use local dojo.
	_d.query("option", item).forEach(function(opt){
      
       ....

comment:17 Changed 10 years ago by dante

@les - FYI I changed xhr.js this morning actually to use _d.config. We use dojo.somethingSomething = function() because the doc parser used to not be able to disambiguate the d -> dojo alias in a local scope. I _believe_ this is resolved, but it the updates haven't made it across the board.

comment:18 Changed 10 years ago by bill

Milestone: 1.41.5

I'm not sure that making locals for dojo and dijit would actually help any of the widgets, since I don't remember any that access dojo/dijit in a loop. It would be interesting to check though, to see if (for example) themeTester.html loads any faster after adding a closure for all the widgets. If it doesn't, then probably want to wait until 2.0 when we are talking about changing the whole dojo.require() system, so that every included module would get a local variable, something like this:

dojo(["dojo", "dijit"], function(dojo, dijit){ .... })

About the changes for manager.js, they sounds reasonable, will look at that soon but beta is approaching so I want to focus on actual bugs for now. I'll mark this for 1.5.

PS: agreed about the inconsistency in the global to local shadowing in dojo core.

comment:19 Changed 10 years ago by bill

Type: enhancementtask

seems like these aren't really enhancements from the users' point of view, labeling as tasks instead

comment:20 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [20981]) More performance tweaks for manager.js, fixes #9915 !strict.

Needed to tweak robot too so that dijit.byId() in the robot test files will still find widgets w/in the iframe (that loads the URL).

comment:21 Changed 10 years ago by bill

My checkin above tweaks the dijit utility functions to use a hash[] variable directly. I also put in the dojo/dijit local variables thing although that doesn't seem as important as neither of those variables is accessed very often.

I'm not going to convert all the widgets to have the function wrapper as that's something we'll likely do for 2.0 when the require/provide mechanism is refactored.

Please open other tickets if there are more performance tweaks as this ticket has become very long.

comment:22 Changed 10 years ago by bill

(In [20983]) Now that dijit/robotx.js makes dijit (in the main window) point to dijit in the <iframe>, it's neither necessary nor possible to load a second copy of dijit into the main window. Doing so causes page unload handlers fire twice, among possible other problems too. Refs #9915 !strict.

comment:23 Changed 10 years ago by bill

(In [21013]) Lots of test file cleanup, plus fixes related to using dijit.editor._selection given there are two copies of dojo simultaneously loaded (one in Editor_fontChoice.html and one in test_FontChoice.html), but just one copy of dijit (in test_FontChoice.html). Refs #9377, #9915.

comment:24 Changed 10 years ago by bill

(In [21392]) Hack to get FocusManager.html automated test running again, refs #9915.

comment:25 Changed 10 years ago by bill

(In [21393]) Get slider test on IE running again, I think there's a problem where two separate instances of dojo connect to the same event on the same node, so it's better to use the same copy of dojo. Refs #9915.

comment:26 Changed 10 years ago by bill

Resolution: fixed
Status: closedreopened

I like the local dojo/dijit variable idea in [20981], but unfortunately breaks the doc parser: it doesn't see dijit.byId() (although it still sees dijit.WidgetSet.byId()).

The parser can't handle this:

(function(dojo, dijit){
   dijit.byId = ...
})(dojo, dijit);

although it can handle this:

(function(){
   dijit.byId = ...
})();

Tested by:

$ cd util/docscripts
$ php generate.php dijit
$ grep "Returns a widget by it's id, or if passed a widget, no-op" cache/api.json

I could use /*===== ... =====*/ comments for every method to make it work, or attempt to fix the doc parser, but I think that any speed optimization should be by using local variables for functions inside of dojo/dijit, rather than dojo/dijit variables themselves, for example:

var style = dojo.style;

so I'll change it that way.

comment:27 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [21460]) Refactor code so that doc parser works again (picking up dijit.byId() etc.). As a bonus I think this will actually run faster than before. Fixes #9915 !strict.

Note: See TracTickets for help on using tickets.