Opened 6 years ago

Closed 5 years ago

#17515 closed enhancement (fixed)

Performance issue due to use of exception to control flow in dojo/selector/acme qsa impl

Reported by: Chris Mitchell Owned by: bill
Priority: undecided Milestone: 1.11
Component: Query Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

[reported by customer] In dojo/selector/acme.js, in the _queryFuncCacheQSA function, we see the following code (we use version 1.8.3, but this code has not changed in the latest version, 1.9.x)

try{
if(!((9 == root.nodeType) || nospace)){ throw ""; }
var r = root[qsa](tq);
// skip expensive duplication checks and just wrap in a NodeList
r[noZip] = true;
       return r;
}catch(e){
// else run the DOM branch on this query, ensuring that we
// default that way in the future
return getQueryFunc(query, true)(root);
}

This is using an exception to control flow, which apart from being poor style, is a definite performance no-no, and it noticeably impacts pages with many selectors. I changed the code in our app to eliminate the throw and this simple change makes a 10% difference (~150ms) in rendering the page, which is using selectors extensively. I can easily fix this in our source base, but it should be changed in Dojo, otherwise we’ll have to keep porting the change as we upgrade.

Change History (11)

comment:1 Changed 6 years ago by bill

Component: CoreQuery
Description: modified (diff)
Owner: set to Kris Zyp
Type: defectenhancement

I agree it's poor style, but OTOH acme.js is deprecated code and it should only be running on IE8. Otherwise dojo/query will use selector/lite rather than selector/acme. Which browser is the customer using?

comment:2 Changed 6 years ago by Chris Mitchell

Android (default Android Browser via Webview, not Chromium) iOS Safari 5-7 investigating for future IE10/Surface Win8 RT

Not sure why they're using acme, will see if they can switch off.

Thanks for the info.

comment:3 Changed 6 years ago by Chris Mitchell

FYI. It seems this module not documented as deprecated clearly, so people are still using it and no warning flags have been seen.

Neither of these doc pages indicate that selectors/acme is deprecated. http://dojotoolkit.org/reference-guide/1.9/dojo/index.html#dojo-index http://dojotoolkit.org/reference-guide/1.9/dojo/query.html#dojo-query (selector engines: acme)

comment:4 Changed 6 years ago by bill

I suspect the problem is that they are accessing dojo.query() through the dojo global, rather than using AMD, require(["dojo/query" ....

So ACME is sort of implied being deprecated by the move to AMD.

I don't care if you remove the try/catch, but I just think it will be better for the customer to switch to the lite engine because they'll be downloading less code to the browser so the page will load faster.

Last edited 6 years ago by bill (previous) (diff)

comment:5 Changed 6 years ago by Chris Mitchell

Use within the app was via AMD, not dojo global: define([ …

"dojo/query",

query selectors

], function(…query, …) {

query(".selector, .selectorRightArrow", this.selectorParent).forEach(function(node) {

Will advise to move to lite.

comment:6 Changed 6 years ago by dylan

If the app needs CSS3 selectors in IE 6-8, lite won't suffice. It would be better to fix the issue, since 1.8.x is supposed to support back to IE6.

comment:7 Changed 6 years ago by dylan

Keep in mind, if dojo/_base/browser is somehow pulled in, even in AMD/async mode, acme will still be pulled in by default.

Also, I do not recall an official acme deprecation warning, nor do I really think we can have one for 1.x, since we're trying to support browsers earlier than IE9 in the 1.x line of code. lite is not sufficient for IE8 users (or IE 6/7 of course).

We cannot retroactively tell people that a feature in 1.8 is backported in 1.9, and thus should not be fixed.

Also, I still don't agree with how we quietly dropped IE 6/7 support for 1.9. I agree that certain features may not support those browsers, but we had made a commitment to our users that 1.x was for old versions of IE, and 2.x would not support them.

comment:8 Changed 6 years ago by Kris Zyp

Can you describe more precisely what change is being proposed? I would have assumed that this is try/catching errors thrown both from the throw "" statement as well as those coming from the native qSA call. It is clear how you directly call getQueryFunc if the first if conditions matches, but can we really eliminate try/catching the qSA? Isn't there selector syntax that is not supported by IE8, that Acme handles by checking for errors thrown here? I could be wrong about that, but just curious if you have a specific change to this code that will pass the unit tests.

comment:9 Changed 5 years ago by bill

#18508 is a duplicate of this ticket.

comment:10 Changed 5 years ago by bill

Milestone: tbd1.11
Owner: changed from Kris Zyp to bill
Status: newassigned

I've got a fix and will check it in.

comment:11 Changed 5 years ago by bill

Resolution: fixed
Status: assignedclosed

Fixed in d55c440c02a3800159becc9ffbef92911d713354.

Note: See TracTickets for help on using tickets.