Opened 10 years ago

Closed 10 years ago

#10174 closed defect (invalid)

Remove "this." references in bootstrap.js when defining dojo/dijit/dojox

Reported by: James Burke Owned by: James Burke
Priority: high Milestone: 1.4
Component: Core Version: 1.4.0b
Keywords: Cc:
Blocked By: Blocking:

Description

Bill found that removing the "this" on this.dojo = {}, there was a noticeable performance improvement in IE and Firefox. The "this" references are from legacy code and do not need to be there now.

Be sure to do a multiversion test after doing the change.

Change History (11)

comment:1 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [20631]) Fixes #10174, remove this references for better performance. Fixed a multiversion test. \!strict

comment:2 Changed 10 years ago by bill

This is actually a dup of #9114, meaning that this is the problem we were trying to solve there. So I'll close that ticket too.

comment:3 Changed 10 years ago by davidmark

Resolution: fixed
Status: closedreopened

The thinking here is that undeclared globals will resolve faster? I would be cautious with that interpretation. Undeclared globals are known to cause inconsistent cross-browser behavior as well.

I want to review the tests and evidence gathered in support of this theory. Should be suspicious of such evidence as it doesn't indicate what will happen in future versions of supported browsers, other configurations of supported browsers, other platforms, etc.

var dojo;

dojo = dojo || { ... };

Ultimately, I would concentrate on removing references to the global dojo object:-

var byId = dojo.byId;

...

var el = byId('myelement');

That strategy results in all local references and eliminates dot operations. I've found that is the best way to maximize performance for methods of global objects. This is the technique I used in the branch (when possible).

comment:4 Changed 10 years ago by davidmark

I knew there was at least one good reason (other than style).

<html>
<head>
<title>Always declare global variables</title>
<script type="text/javascript">
window.onload = function() {
    window.alert(dojo);
    dojo = {};
    window.alert(dojo);
};
</script>
</head>
<body>
<div id="dojo"></div>
</body>
</html>

Run this in the supported browsers and the problem is apparent. Declare dojo and the problem goes away. This is but one problem for one identifier. Dropping declarations for all Dojo global variables is tempting fate.

comment:5 Changed 10 years ago by bill

I want to review the tests and evidence gathered in support of this theory.

Please go ahead, you can do that on your own. See #9114, the test is taskspeed. See also http://thread.gmane.org/gmane.comp.web.dojo.devel/11678.

Ultimately, I would concentrate on removing references to the global dojo object... This is the technique I used in the branch (when possible).

This change is to make user code that references dojo faster, not to make dojo internally faster. We can't change users' code since it's not in dojo's SVN.

comment:6 in reply to:  4 ; Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

Replying to davidmark:

Run this in the supported browsers and the problem is apparent. Declare dojo and the problem goes away. This is but one problem for one identifier. Dropping declarations for all Dojo global variables is tempting fate.

It would be helpful if you know the results to include them in the bug report instead of requiring more time to be spent by someone else to reproduce.

For future reference, the problem browser is IE. I used the following test:

<html>
<head>
<title id="dojo">Always declare global variables</title>
<script type="text/javascript">
(function(){
dojo = {
  color: "blue"
};
})();
window.onload = function() {
    window.alert(dojo.color);
};
</script>
</head>
<body>
<div id="dojo"></div>
</body>
</html>

Basically, if an element with ID of dojo is created before dojo is defined in script, there will be an error in IE when trying to define dojo as an object in script. I used IE 8 to test this. Firefox 3.5 and Safari 4 do not care, they create dojo as the object specified in script.

I believe it highly unlikely that someone will create a node called dojo, dijit or dojox. If they do, it is just global namespace collision, and we can inform them of that. Right now the speed advantage in IE is worth it, as Bill points out, not only for us but for outside code using dojo.

If there are other issues, it would be good to know, but closing for now.

comment:7 in reply to:  6 Changed 10 years ago by davidmark

Resolution: fixed
Status: closedreopened

Replying to jburke:

Replying to davidmark:

Run this in the supported browsers and the problem is apparent. Declare dojo and the problem goes away. This is but one problem for one identifier. Dropping declarations for all Dojo global variables is tempting fate.

It would be helpful if you know the results to include them in the bug report instead of requiring more time to be spent by someone else to reproduce.

For future reference, the problem browser is IE. I used the following test:

<html>
<head>
<title id="dojo">Always declare global variables</title>
<script type="text/javascript">
(function(){
dojo = {
  color: "blue"
};
})();
window.onload = function() {
    window.alert(dojo.color);
};
</script>
</head>
<body>
<div id="dojo"></div>
</body>
</html>

Basically, if an element with ID of dojo is created before dojo is defined in script, there will be an error in IE when trying to define dojo as an object in script. I used IE 8 to test this. Firefox 3.5 and Safari 4 do not care, they create dojo as the object specified in script.

Or a name of dojo or dijit or dojox, etc. Remember that dojo is both a common noun and a proper name in English and that Dojo is used all over the world. It sounds as if we are making a major behavior change at the last second.

I believe it highly unlikely that someone will create a node called dojo, dijit or dojox. If they do, it is just global namespace collision, and we can inform them of that. Right now the speed advantage in IE is worth it, as Bill points out, not only for us but for outside code using dojo.

These namespaces are supposed to prevent collisions (and this is about the worst collision possible). This is a very hard to track down bug. Imagine a user develops their app against standards-based browsers first, then tries it in IE, only to be presented with an error and a blank page. Imagine an app that adds a form with an element named "dojo" and suddenly breaks IE. Imagine there are already apps out there with collisions waiting to happen. There's no rule that says the Dojo script has to go in the head.

Furthermore, I'm not sold on the evidence at all. I don't think it says what it is interpeted as saying. And I think it is more direct to address the number of times that the globals are referenced in each module (can be reduced to very few).

If there are other issues, it would be good to know, but closing for now.

I don't think we can have it both ways. On one hand, the slightest behavior change is scrutinized (e.g. loading hacks). On the other, anything goes. We need more input on this from the other contributors.

comment:8 Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

No new information provided in your comment. Closing as fixed. We are having another beta release of the code, so we can also wait to get feedback on the change during that time, to get better real data.

As mentioned before, the fix is to help others code in addition to internal code. Internal code using variables is fine to want to do, but that should be a different ticket.

If you are not sold on the evidence, then more data is needed, but please wait to have the data before reopening.

If you prefer to take your case to more contributors, you may want to try on the mailing list. Not enough watch all changes to the tickets to get a good sample. Of course, any other contributors watching the ticket are free to comment.

comment:9 Changed 10 years ago by bill

We need more input on this from the other contributors.

Please remember that James is in charge of core so he doesn't need approval from you (or other contributors) to make a change.

Remember that dojo is both a common noun and a proper name in English

For the record though, I'm also not worried at all about someone declaring a node with id="dojo", or id="dijit". Of course anything is possible, perhaps if someone was using dojo to design a website about dojos, but even then I don't see it happening.

comment:10 in reply to:  9 Changed 10 years ago by davidmark

Resolution: fixed
Status: closedreopened

Replying to bill:

We need more input on this from the other contributors.

Please remember that James is in charge of core so he doesn't need approval from you (or other contributors) to make a change.

Of course, the data posted by you in the mailing list seems to show that this is a hair faster:-

var dojo;

Which is what I have been suggesting all along (and the way it appears in the branch). And it won't cause the aforementioned name space collisions, which are present not only in IE, but in other supported browsers that mimic some of IE's quirks in quirks mode.

So what was the argument for un-declaring our globals? Where is the data that backs it up?

comment:11 Changed 10 years ago by davidmark

Resolution: invalid
Status: reopenedclosed

I suggest a new ticket. As noted, this is the proper (and optimal as it turns out) form:-

var dojo;

A solution involving implied (undeclared) global variables should never have been considered.

Note: See TracTickets for help on using tickets.