Use browsers JSON.parse for parsing JSON.

Use browsers JSON.parse for parsing JSON.

This shrinks swarm by 27k (3.6%)
We can't make the error messages be the same since the native error contains only a message.

Committed: https://code.google.com/p/dart/source/detail?r=18021

[sra1, 2013-01-31 05:02:59]

Addresses Issue 4653

[Lasse Reichstein Nielsen, 2013-01-31 06:58:44]

It looks like a good solution. I would probably have tried using the reviver
argument to JS's JSON.parse to convert objects (and failed, since it's
impossible to pass integer indices to the reviver for lists), and walking it
afterwards is much simpler.

LGTM if the dart2js people accept it.

How is speed?

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:1: // Patch file
for dart:json library.
Needs copyright header.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:5: part of
dart.json;
Does this parse? You usually can't have "part of" in the same file as an
"import". Maybe it's just ignored for patch-files, but it still looks odd.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:23: patch
parse(String json, [reviver(var key, var value)]) {
You can't patch a function that isn't marked external.
(Or, if you can, it's a bug, so please report it and stop doing it :).
I.e., you must mark the method external in the JSON library and move the
original implementation to the VM's patch file.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:43: walk(e) {
Here "e" is really a JS object, not a Dart object, so maybe allow yourself to
do:
  if (e === null || !JS("bool", 'typeof # == "object"', e)) {
    return e;
  }

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:56: var keys =
JS('=List', 'Object.keys(#)', e);
Notice that V8 still has a bug handling keys named "__proto__"
(<https://code.google.com/p/v8/issues/detail?id=621>, +mstarzinger).
That means that '{"__proto__": {"x": 42}, "y": 37}' collapses to '{"x":42,
"y":37}' using the JS parse, but worked with the Dart one.

Probably not something that happens in practice, though.

[Lasse Reichstein Nielsen, 2013-01-31 07:01:40]

CC'ing mstarzinger too, as I said I would.
Is there any plan for JSON __proto__ handling? E.g., going to the Firefox
__proto__ behavior?

[Johnni Winther, 2013-01-31 08:32:59]

lgtm

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:5: part of
dart.json;
On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> Does this parse? You usually can't have "part of" in the same file as an
> "import". Maybe it's just ignored for patch-files, but it still looks odd.

Yes, it looks odd. It should be removed.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:14: * The optional
[revivier] function, if provided, is called once for each object
revivier -> reviver

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:17: * the revivier
will be used as the value of that property instead the parsed
ditto

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:23: patch
parse(String json, [reviver(var key, var value)]) {
On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> You can't patch a function that isn't marked external.
> (Or, if you can, it's a bug, so please report it and stop doing it :).
> I.e., you must mark the method external in the JSON library and move the
> original implementation to the VM's patch file.

There is a pending CL that enforces this.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart
File sdk/lib/json/json.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart#new...
sdk/lib/json/json.dart:44: * The optional [revivier] function, if provided, is
called once for each object
revivier -> reviver

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart#new...
sdk/lib/json/json.dart:47: * the revivier will be used as the value of that
property instead the parsed
ditto

[sra1, 2013-01-31 22:49:36]

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:1: // Patch file
for dart:json library.
On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> Needs copyright header.

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:5: part of
dart.json;
On 2013/01/31 08:32:59, Johnni Winther wrote:
> On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> > Does this parse? You usually can't have "part of" in the same file as an
> > "import". Maybe it's just ignored for patch-files, but it still looks odd.
> 
> Yes, it looks odd. It should be removed.

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:14: * The optional
[revivier] function, if provided, is called once for each object
On 2013/01/31 08:32:59, Johnni Winther wrote:
> revivier -> reviver

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:17: * the revivier
will be used as the value of that property instead the parsed
On 2013/01/31 08:32:59, Johnni Winther wrote:
> ditto

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:23: patch
parse(String json, [reviver(var key, var value)]) {
On 2013/01/31 08:32:59, Johnni Winther wrote:
> On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> > You can't patch a function that isn't marked external.
> > (Or, if you can, it's a bug, so please report it and stop doing it :).
> > I.e., you must mark the method external in the JSON library and move the
> > original implementation to the VM's patch file.
> 
> There is a pending CL that enforces this.

Which CL is that?
Please don't commit until I have solved the my current problem.

Adding 'external' requires I have a patch in the VM too.

I'm blocked on the VM side, and seeking help from the VM team.
At this point it looks like a simple change like this requires writing C++ code
and changing multiple VM files. We should improve the 'patch' feature to be more
usable if that is true.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:43: walk(e) {
On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> Here "e" is really a JS object, not a Dart object, so maybe allow yourself to
> do:
>   if (e === null || !JS("bool", 'typeof # == "object"', e)) {
>     return e;
>   }

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:56: var keys =
JS('=List', 'Object.keys(#)', e);
On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> Notice that V8 still has a bug handling keys named "__proto__"
> (<https://code.google.com/p/v8/issues/detail?id=621>, +mstarzinger).
> That means that '{"__proto__": {"x": 42}, "y": 37}' collapses to '{"x":42,
> "y":37}' using the JS parse, but worked with the Dart one.
> 
> Probably not something that happens in practice, though.

Interesting.
Object.keys(o) for this example returns ["y"]
So this code will completely drop the __proto__, giving a third behavior.  I'll
add a comment about the issue in case it becomes something in practice.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart
File sdk/lib/json/json.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart#new...
sdk/lib/json/json.dart:44: * The optional [revivier] function, if provided, is
called once for each object
On 2013/01/31 08:32:59, Johnni Winther wrote:
> revivier -> reviver

Done.

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/json/json.dart#new...
sdk/lib/json/json.dart:47: * the revivier will be used as the value of that
property instead the parsed
On 2013/01/31 08:32:59, Johnni Winther wrote:
> ditto

Done.

[sra1, 2013-01-31 23:03:31]

Adding Siva to review the 8 files needed to put the patch in the VM.

[siva, 2013-01-31 23:16:40]

I don't see any json_patch.dart file added to the bin directory.

Your comment is also incorrect you are not adding this to the VM you are adding
it to the standalone Dart program.

https://codereview.chromium.org/12114021/diff/10001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/12114021/diff/10001/runtime/bin/bin.gypi#newc...
runtime/bin/bin.gypi:235: '../lib/json_sources.gypi',
Where is this file, I don't see it in the CL.

[sra1, 2013-02-01 01:19:03]

Added missing files.  PTAL.

https://codereview.chromium.org/12114021/diff/10001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://codereview.chromium.org/12114021/diff/10001/runtime/bin/bin.gypi#newc...
runtime/bin/bin.gypi:235: '../lib/json_sources.gypi',
On 2013/01/31 23:16:40, siva wrote:
> Where is this file, I don't see it in the CL.

I have added this and moved from lib to bin.

[Johnni Winther, 2013-02-01 06:20:10]

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:23: patch
parse(String json, [reviver(var key, var value)]) {
On 2013/01/31 22:49:36, sra1 wrote:
> On 2013/01/31 08:32:59, Johnni Winther wrote:
> > On 2013/01/31 06:58:44, Lasse Reichstein Nielsen wrote:
> > > You can't patch a function that isn't marked external.
> > > (Or, if you can, it's a bug, so please report it and stop doing it :).
> > > I.e., you must mark the method external in the JSON library and move the
> > > original implementation to the VM's patch file.
> > 
> > There is a pending CL that enforces this.
> 
> Which CL is that?
> Please don't commit until I have solved the my current problem.
> 
> Adding 'external' requires I have a patch in the VM too.
> 
> I'm blocked on the VM side, and seeking help from the VM team.
> At this point it looks like a simple change like this requires writing C++
code
> and changing multiple VM files. We should improve the 'patch' feature to be
more
> usable if that is true.

The CL is https://codereview.chromium.org/11864010/. I'll hold it back until you
commit. 'external' is currently only enforced on members but not on top level
functions. The CL is meant to fix that.

[ngeoffray, 2013-02-01 08:49:12]

dart2js changes LGTM

https://codereview.chromium.org/12114021/diff/16001/runtime/bin/json_patch.dart
File runtime/bin/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/16001/runtime/bin/json_patch.da...
runtime/bin/json_patch.dart:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
2012 -> 2013

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:1: // Copyright
(c) 2012, the Dart project authors.  Please see the AUTHORS file
2012 -> 2013

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:10: * Parses
[json] and build the corresponding parsed JSON value.
build -> builds

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:19: * the reviver
will be used as the value of that property instead the parsed
instead *of*

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:31: } catch (e) {
`e` not being a Dart exception, I wonder how well dart2js handles this. What's
the JS type of the exception? Is String(#) sufficient enough to make it dartj2s
friendly?

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:49: if (e == null
|| JS('bool', 'typeof # != "object"', e)) {
Since `e` can be anything, it's safer to use JS(bool, '# == null'). Otherwise
you rely on the compiler to emit an inlined null check instead of calling a
method on e.

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:53: // bug 621
below is fixed.
Why is the getPrototypeOf needed?

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/json/json.dart
File sdk/lib/json/json.dart (right):

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/json/json.dart#ne...
sdk/lib/json/json.dart:47: * the reviver will be used as the value of that
property instead the parsed
instead *of*

[Lasse Reichstein Nielsen, 2013-02-01 12:33:32]

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:67: String key =
keys[i];
Keys also gets property names from the prototype, so if __proto__ was set, you
get those elements too.
Either check for "e.hasOwnProperty(key)" or (preferably), check for the
__proto__ property first and remove it before getting keys. That makes the
expensive operations only happen if __proto__ is set.

[Michael Starzinger, 2013-02-01 13:40:05]

On 2013/01/31 07:01:40, Lasse Reichstein Nielsen wrote:
> CC'ing mstarzinger too, as I said I would.
> Is there any plan for JSON __proto__ handling? E.g., going to the Firefox
> __proto__ behavior?

AFAIK, the standardization of __proto__ is still being discussed by TC39
(Andreas might know more), we want to hold off with changing JSON's handling
until __proto__ semantics have been settled. There are two V8 issues that track
this:

https://code.google.com/p/v8/issues/detail?id=621
https://code.google.com/p/v8/issues/detail?id=1949

[sra1, 2013-02-01 21:52:43]

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:10: * Parses
[json] and build the corresponding parsed JSON value.
On 2013/02/01 08:49:12, ngeoffray wrote:
> build -> builds

Done.

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:19: * the reviver
will be used as the value of that property instead the parsed
On 2013/02/01 08:49:12, ngeoffray wrote:
> instead *of*

Done.

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:49: if (e == null
|| JS('bool', 'typeof # != "object"', e)) {
On 2013/02/01 08:49:12, ngeoffray wrote:
> Since `e` can be anything, it's safer to use JS(bool, '# == null'). Otherwise
> you rely on the compiler to emit an inlined null check instead of calling a
> method on e.

Done.

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:53: // bug 621
below is fixed.
On 2013/02/01 08:49:12, ngeoffray wrote:
> Why is the getPrototypeOf needed?

Done.

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:67: String key =
keys[i];
On 2013/02/01 12:33:32, Lasse Reichstein Nielsen wrote:
> Keys also gets property names from the prototype, so if __proto__ was set, you
> get those elements too.
> Either check for "e.hasOwnProperty(key)" or (preferably), check for the
> __proto__ property first and remove it before getting keys. That makes the
> expensive operations only happen if __proto__ is set.

"Returns an array of a given object's own enumerable properties, in the same
order as that provided by a for-in loop (the difference being that a for-in loop
enumerates properties in the prototype chain as well)."
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/...

On V8,
Object.keys(JSON.parse('{"__proto__":{"X":42},"Y":34}'))
-->
["Y"]

[Lasse Reichstein Nielsen, 2013-02-02 11:16:23]

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/json_patch.dart (right):

https://codereview.chromium.org/12114021/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/json_patch.dart:67: String key =
keys[i];
Ack, my bad.

[siva, 2013-02-05 23:55:57]

Is this change committed?

There was no LGTM from the VM side, we were still discussing the issues with
Dartium.

[siva, 2013-02-05 23:59:00]

None of the patching stuff is needed for the VM side as you are just redirecting
it back to the original _patch function.

I don't see anything new being done which is VM specific.

[sra1, 2013-02-06 00:02:53]

On 2013/02/05 23:55:57, siva wrote:
> Is this change committed?
> 
> There was no LGTM from the VM side, we were still discussing the issues with
> Dartium.

It was committed and rolled back as soon as the interesting bots had started to
use the bots as a 'try-bot'.

[sra1, 2013-02-06 00:18:43]

On 2013/02/05 23:59:00, siva wrote:
> None of the patching stuff is needed for the VM side as you are just
redirecting
> it back to the original _patch function.
> 
> I don't see anything new being done which is VM specific.

All I want to do is provide a different version for dart2js.

Here is my understanding of the patch mechanism.  I hope I am wrong about
something.

1. You can only patch a function declared as 'external'.

2. An external function may not have a body.

3. 'parse' has to be external (and have no body) since there are different
dart2js and vm implementations.

4. since 'parse' is external, there needs to be a 'patch parse'.  This function
is different for dart2js and vm, the dart2js one uses inline JS code, the vm one
calls '_parse'.

5. Where does the vm 'patch parse' function live?  I created a one-line
runtime/bin/json_patch.dart which I have had difficulty integrating with
Dartium.

[siva, 2013-02-06 00:24:32]

I believe 'Item 3' is an incorrect interpretation of the rules.
It should be possible to patch without having to declare 'parse' as an external
function with no body.

On 2013/02/06 00:18:43, sra1 wrote:
> On 2013/02/05 23:59:00, siva wrote:
> > None of the patching stuff is needed for the VM side as you are just
> redirecting
> > it back to the original _patch function.
> > 
> > I don't see anything new being done which is VM specific.
> 
> All I want to do is provide a different version for dart2js.
> 
> Here is my understanding of the patch mechanism.  I hope I am wrong about
> something.
> 
> 1. You can only patch a function declared as 'external'.
> 
> 2. An external function may not have a body.
> 
> 3. 'parse' has to be external (and have no body) since there are different
> dart2js and vm implementations.
> 
> 4. since 'parse' is external, there needs to be a 'patch parse'.  This
function
> is different for dart2js and vm, the dart2js one uses inline JS code, the vm
one
> calls '_parse'.
> 
> 5. Where does the vm 'patch parse' function live?  I created a one-line
> runtime/bin/json_patch.dart which I have had difficulty integrating with
> Dartium.

[kasperl, 2013-02-06 07:07:11]

https://codereview.chromium.org/12114021/diff/17001/sdk/lib/json/json.dart
File sdk/lib/json/json.dart (right):

https://codereview.chromium.org/12114021/diff/17001/sdk/lib/json/json.dart#ne...
sdk/lib/json/json.dart:55: _parse(String json, [reviver(var key, var value)]) {
If this code is only used by the VM, it shouldn't live in the
platform-independent part of the library. That also goes for all the
implementation code below I guess.

[sra1, 2013-02-06 17:22:39]

https://codereview.chromium.org/12114021/diff/17001/sdk/lib/json/json.dart
File sdk/lib/json/json.dart (right):

https://codereview.chromium.org/12114021/diff/17001/sdk/lib/json/json.dart#ne...
sdk/lib/json/json.dart:55: _parse(String json, [reviver(var key, var value)]) {
On 2013/02/06 07:07:11, kasperl wrote:
> If this code is only used by the VM, it shouldn't live in the
> platform-independent part of the library. That also goes for all the
> implementation code below I guess.

The code can still be used by someone who needs their own custom JsonListener.
dart2js is smart enough to leave it out for 'normal' usage.

I intend to write a benchmark that uses JsonParser and the provided JsonListener
implementations to track dart2js compiled code performance which is currently
similar to VM performance.  Both are currently slower than the browser native
parser.