Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#9131 closed defect (fixed)

dojo.isString is slow - and often-called

Reported by: Nathan Toone Owned by: Nathan Toone
Priority: high Milestone: 1.4
Component: General Version: 1.3.0
Keywords: Cc: 7twenty
Blocked By: Blocking:

Description

Since all the browsers that we support will behave in the same way be replacing

dojo.isString(var);

with

typeof var == "string"

we can probably improve performance quite a bit (especially on IE) by removing the overhead of the extra function call in our code base.

Change History (9)

comment:1 Changed 11 years ago by bill

Hmm, if

typeof var == "string"

is all that's needed then why is the expression in dojo.isString() so complicated?

!!arguments.length && it != null && (typeof it == "string" || it instanceof String); // Boolean

comment:2 Changed 11 years ago by Nathan Toone

I have absolutely no clue.

The !!arguments.length would take care of passing in no parameters (ie calling "dojo.isString()") - which I don't see us doing anywhere in the code. it != null doesn't really help much - since typeof null is "object" and null is not an instanceof String. Also - since it's != instead of !==, some browser will treat

"" != null

as true, I believe - thus opening up a bug.

The typeof
instanceof check is so that you can pass
dojo.isString("abc");

or

dojo.isString(new String("abc"));

which, I'm not seeing us use any wrapper object in any of our code (and if anyone is, they probably shouldn't be - for performance reasons).

All in all, I don't see why this function couldn't be distilled into just returning

typeof == "string";

But you can avoid the overhead of the function (quite significant in certain browsers....*cough* IE *cough*) by just calling typeof instead of dojo.isString.

I was able to take a grid which was making 2000 calls to dojo.isString down to making 1000 calls, just by replacing the single dojo.isString in dojo.byId. I was further able to drop it another 500 calls by replacing the dojo.isString in dojo.parser.

comment:3 Changed 11 years ago by Nathan Toone

Removed extraneous checks in dojo.isString as initial step... [17293]

comment:4 Changed 11 years ago by Nathan Toone

(In [17295]) Refs #9131 - remove extraneous boolean checks for dojo.isString()

comment:5 Changed 10 years ago by alex

(In [17969]) Inline isString checks in dojo.js to boost performance.

!strict

Refs #9131

comment:6 Changed 10 years ago by bill

(In [20107]) Dijit performance optimizations from Les Szklanny (CLA on file), thanks! Fixes #9885, #9875, #9879, refs #9131. !strict

comment:7 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: newclosed

Closing as fixed - as the most glaring cases have been addressed.

comment:8 Changed 10 years ago by bill

Note that this breaks user code that (for some reason) does something like:

var id = new String("myNodeId");
dojo.style(myNodeId, ...)

Basically we are saying that from 1.4 onwards we don't support that. Hopefully none of our users are using the String() constructor. Although I've seen a lot of people do new Array() so I wouldn't be too surpised if someone is doing new String().

comment:9 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

Note: See TracTickets for help on using tickets.