Opened 11 years ago

Closed 10 years ago

#9114 closed enhancement (fixed)

crazy taskspeed numbers on IE vs. "pure-dom" version

Reported by: alex Owned by: alex
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: performance, taskspeed, ie, ie6, ie7 Cc: jamestag, dante, wildbill
Blocked By: Blocking:

Description (last modified by Eugene Lazutkin)

There's something really funky going on with our TaskSpeed results on IE 6. It seems that calls like "dojo.byId" are going incredibly slowly when defined inside of Dojo, but when referenced with local aliases, they speed up dramatically. This is most pronounced on the addClass-odd test. The cause deserves more investigation, but the short-term outcome is that creating local aliases for things which don't need "this" references internally can speed up IE dramatically.

Change History (14)

comment:1 Changed 11 years ago by Eugene Lazutkin

Description: modified (diff)

Probably you forgot that Bill complained about a similar problem (or maybe this very problem) during the last dojo meeting. According to him there is a measurable difference between using properties on dojo and mydojo, where the latter is defined something like this:

// let's copy dojo
var mydojo = dojo.mixin({}, dojo);

My uneducated guess was that while building dojo a hash table underneath got overflown several times leading to a degraded structure (e.g., multiple linear lists to hold chunks, overflown hash chains, or excessive holes). Apparently the problem is solved by copying, which leads to some compacting under the hood.

I promised to investigate the problem and get some solid benchmark data first. It looks like you already started on the path to the solution. Let me know if you want to investigate it on your own, so I'll work on something else from my plate.

comment:2 Changed 11 years ago by Eugene Lazutkin

Description: modified (diff)

I re-read the description and (finally!) noticed your reference to addClass, which was the example used by Bill. So you do remember, but apparently I cannot read texts longer than 5 words.

Still my invitation stands. ;-)

comment:3 Changed 11 years ago by dante

this might explain why plugd appeared faster in some IE tests of identical functionality. it makes several local references and shares them througout the one file. eg:

var d = dojo, place = d.place, style = d.style

comment:4 Changed 11 years ago by dante

On a hunch, I threw "MooJo?" into taskspeed, running a modified version of the dojo-tests.js tests (MooJo? exports dojo.* public api's onto window[]). MooJo?'s time in IE6/VM: 2696 vs Dojo 1.2.3: 5149 and Dojo 1.3.0: 4499 (pureDom:961)

MooJo? tests are just reduced versions of the 1.3 tests:

	"bindattr" : function(){
		var someFn = function(){};
		return query("ul > li").forEach(function(n){
			var c = connect(n, "onmouseover", someFn);
			attr(n, "rel", "touched");
			disconnect(c);
		}).length;
	},

http://dante.dojotoolkit.org/static/otherlibs/moojo.html

not suggesting this is a good idea, but a similar copying of the dojo namespace occurs here and is interesting to see.

comment:5 Changed 11 years ago by bill

Here's the original thread (which we later discussed at the meeting).

If you notice my test file it's not even calling dojo.byId() (directly or indirectly) so I'm not sure how changing dojo.byId() could affect the performance.

comment:6 Changed 11 years ago by bill

[17266] is Alex's fix to dojo.byId(), as usual it didn't get auto-updated here.

comment:7 Changed 11 years ago by bill

I just benchmarked the dojo.byId() change. Thanks for checking that in, it does fix the addclass-odd 10x performance discrepancy.

However, as my modified tests show (and as Eugene stated above) there's a problem with the dojo Object itself, which [17266] is just working around for a common case. Would be nice to fix the root problem as even after the above check-in code like this is still slow:

for(...){
	dojo.addClass(n, "added");
}

(And many users are likely to write code like that.)

One other thing to note is that dojo.byId() shouldn't be getting called at all for dojo.query(...).addClass(). It's just an implementation inefficiency.

comment:8 Changed 11 years ago by bill

[17277] adds unit test for dojo.byId(null), which stopped working with [17266].

comment:9 Changed 11 years ago by Mark Wubben

I'll add that dojo.byId(null) only stopped working in IE and Opera, and this is breaking widget initialization without a source node in those browsers.

comment:10 Changed 11 years ago by Nathan Toone

[17285] provides a fix for the interim - until a more appropriate fix can be placed. This prevents the IE and Opera breakage.

comment:11 Changed 11 years ago by davidmark

The byId method has some inefficiencies, but can be skipped when a node is passed.

if (typeof node == 'string') {
   node = d.byId(node);
}

I updated this in my branch for add/removeClass and changed them to use regular expressions. Previously did more string concatenation, which is relatively slow, especially in IE. Also, spaces aren't the only separators for class names.

As for local references to the dojo object, they may speed things up in some agents (Opera comes to mind), but be very careful as it is easy to create circular references involving DOM nodes. For example, if a local function is attached as a listener:

[dojo]->[doc]->[element]->[listener]->[variable object]->[dojo]

These sorts of circular references are what cause memory leaks in IE.

http://www.jibbering.com/faq/faq_notes/closures.html#clMem

comment:12 Changed 10 years ago by bill

Milestone: 1.4future

comment:13 Changed 10 years ago by bill

(In [20606]) Make dojo.byId(node) on IE work for nodes from other documents again; it stopped working in [17266] (refs #9114). Using typeof check rather than dojo.isString() for performance (refs #9131).

Revert the dojo.destroy() workaround code in [20587] (refs #10107) which is no longer needed.

!strict

comment:14 Changed 10 years ago by bill

Milestone: future1.4
Resolution: fixed
Status: newclosed

Slow access to dojo variable fixed by [20631]: Fixes #10174, remove this references for better performance. Fixed a multiversion test. \!strict

Note: See TracTickets for help on using tickets.