Opened 10 years ago

Closed 9 years ago

#9664 closed defect (wontfix)

featureDetection: eval and fromJson change

Reported by: James Burke Owned by: dylan
Priority: high Milestone: future
Component: Core Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

The featureDetection branch has this for the definition of eval:

dojo["eval"] = function(){
    if (arguments.length == 1 || !arguments[1]) { 
        return eval(arguments[0]);
    }

    // NOTE: Script injection here if post load

    return (new Function(arguments[0]))();
};

I do not see how the return (new Function()) section would ever be called? IIRC, we normally only pass one argument to eval, and doing a quick search under the dojo/ dir in the branch only shows one argument only being passed to eval.

Change History (10)

comment:1 Changed 10 years ago by James Burke

Keywords: davidmark removed
Owner: changed from James Burke to davidmark
Summary: featureDetection: eval changefeatureDetection: eval and fromJson change

A related change, in fromJson:

dojo.fromJson = function(/*String*/ json){
    return (new Function('return (' + json + ')'))(); // Object
};

Bill said he did a quick test using this code:

       var start = new Date();
       for(var i=0; i<10000; i++)
               dojo.fromJson("{a: 1, b: 2}");
       console.log("elapsed: " + (new Date() - start));

and the Function approach is generally slower:

IE6:

old: 594 new: 598

IE7:

old: 594 new: 641

IE8:

old: 219 new: 172

FF3.5/win:

old: 85 new: 165

Safari4/mac:

old: 23 new: 308

FF3.5/mac:

old: 147 new: 3000

This site also found similar results: http://weblogs.asp.net/yuanjian/archive/2009/03/22/json-performance-comparison-of-eval-new-function-and-json.aspx

Are there other benefits to the new Function approach?

At least for the fromJson method, I would prefer to delegate to native JSON.parse if available then just use eval. We have backwards compatibility problem described in #xxx, but generally I favor switching to a new method, maybe dojo.jsonParse() that does the JSON.parse or eval branch based on a capability/feature detection check.

comment:3 Changed 10 years ago by Les

Also, see this jQuery ticket: http://dev.jquery.com/ticket/4680

comment:4 Changed 10 years ago by bill

Les, thanks for finding those links.

I noticed the links from the jquery page saying that firebug scewed the results... the above result for FF3.5/mac had firebug enabled (although the FF3.5/win one didn't), so I reran FF3.5/mac after disabled firebug:

FF3.5/mac:

old: 116 new: 233

The existing method using eval is still faster than using Function.

comment:5 Changed 10 years ago by Les

Looks like the jQuery team picked a slower method. I also checked the latest Ext Core 3.0, and they use eval, not new Function. Both Ext and jQuery check if there's native JSON support, though.

comment:6 Changed 10 years ago by Mike Wilson

There's some discussion in #744 that might be interesting, f ex about debuggability.

comment:7 Changed 10 years ago by Les

At least for the fromJson method, I would prefer to delegate to native JSON.parse if available then just use eval.

I'd recommend adding a configuration option to allow suppressing delegation to native JSON methods even if they are available, i.e. eval would be used even when native JSON methods are available.

comment:8 Changed 10 years ago by James Burke

As bug #8111 points out, there is likely issues with fromJson using native JSON.parse since some code depends on fromJson for some non-json things. So it is more likely we would use new method names and change code where appropriate to use the new methods.

comment:9 Changed 9 years ago by Chris Mitchell

Owner: changed from davidmark to dylan

comment:10 Changed 9 years ago by bill

Resolution: wontfix
Status: newclosed

Closing as per comments above.

Note: See TracTickets for help on using tickets.