Opened 13 years ago
Closed 11 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 13 years ago by
Keywords: | davidmark removed |
---|---|
Owner: | changed from James Burke to davidmark |
Summary: | featureDetection: eval change → featureDetection: eval and fromJson change |
comment:4 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
There's some discussion in #744 that might be interesting, f ex about debuggability.
comment:7 Changed 13 years ago by
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 13 years ago by
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 11 years ago by
Owner: | changed from davidmark to dylan |
---|
comment:10 Changed 11 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Closing as per comments above.
A related change, in fromJson:
Bill said he did a quick test using this code:
and the Function approach is generally slower:
IE6:
IE7:
IE8:
FF3.5/win:
Safari4/mac:
FF3.5/mac:
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.