Make the JSON decoder (parser) create Dart maps lazily

Make the JSON decoder (parser) create Dart maps lazily when no reviver is provided (common case).

R=floitsch@google.com, lrn@google.com
BUG=http://dartbug.com/19649

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

[kasperl, 2014-06-30 13:59:26]

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:138: class _JsonMap implements
Map<String, dynamic> {
Should this be implementing LinkedHashMap<String, dynamic> instead?

[kasperl, 2014-06-30 14:33:01]

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:161: Map<String, dynamic>
_convertToMutableMap() {
We should make sure this is only called if the key set actually changes and then
force an update to the key set in case someone is iterating over it.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:202: return _keys.skip(0);
This can probably be improved by using some sort of sublist iterable.

[floitsch, 2014-06-30 14:49:44]

LGTM with more comments.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:121: var list = JS('JSExtendableArray',
'#', object);  // Teach compiler the type is known.
long line.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:138: class _JsonMap implements
Map<String, dynamic> {
On 2014/06/30 13:59:26, kasperl wrote:
> Should this be implementing LinkedHashMap<String, dynamic> instead?

In theory it's a breaking change if it isn't.
I would prefer simple Map<String, dynamic>, though.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:161: Map<String, dynamic>
_convertToMutableMap() {
On 2014/06/30 14:33:01, kasperl wrote:
> We should make sure this is only called if the key set actually changes and
then
> force an update to the key set in case someone is iterating over it.

I'm not sure it's worth it.
I think json-maps are usually used read-only.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:202: return _keys.skip(0);
On 2014/06/30 14:33:01, kasperl wrote:
> This can probably be improved by using some sort of sublist iterable.

The ConstantMapIterable (_internal/lib/constant_map.dart) has the following
code:

class _ConstantMapKeyIterable<K> extends IterableBase<K> {
  ConstantStringMap<K, dynamic> _map;
  _ConstantMapKeyIterable(this._map);

  Iterator<K> get iterator => _map._keys.iterator;
}

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:232: return (key == '__proto__' &&
JS('bool', '# === Object.prototype', original))
long line.

[Lasse Reichstein Nielsen, 2014-06-30 16:44:15]

lgtm

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:113: if (JS('bool', 'typeof # !=
"object"', object)) {
Add comment say this covers all primitive values.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:138: class _JsonMap implements
Map<String, dynamic> {
No thanks. LinkedHashMap is an implementation class, it should never be used as
an interface. Its interface is indistinguishable from Map, and this should just
be a Map too.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:161: Map<String, dynamic>
_convertToMutableMap() {
Alternatively, allow all String put/add operations to work on the simple object.
When adding a new key, it can be just added at the end of the cached keys list.

https://codereview.chromium.org/336863008/diff/1/sdk/lib/_internal/lib/conver...
sdk/lib/_internal/lib/convert_patch.dart:173: operator[](String key) {
The operator[] takes Object as argument.

[kasperl, 2014-07-01 08:49:25]

I've extended the test coverage and fixed a bunch of issues. This is now ready
for a real review. 

PTAL.

[Lasse Reichstein Nielsen, 2014-07-01 09:43:34]

lgtm

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:42: if (reviver == null) return
_convertJsonToDartLazy(parsed);
Use braces around the then/else parts.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:103: if (JS('bool', '# == null',
object)) {
Do you need the JS call here, or would
   if (object == null) return null;
work as well, because Dart2js treats undefined as null?

That would also allow the compiler to know that object is not null in the
following code.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:107: // JavaScript string, number, bool
are in the correct representation.
are in -> already has

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:114: // bug
https://code.google.com/p/v8/issues/detail?id=621 is fixed.
According to the issue, the bug was fixed more than a year ago. The d8 in
third_party has the fix. I think we can start depending on it.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:134: // the map is eventually upgraded
in which case we null it
',' before in?

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:308: return _setProperty(_processed,
key, result);
This sets the property on _processed, even if it doesn't exist in the original.
I don't think it's visible, but it will keep the key string alive.
I think I'd prefer to not store anything if the key is not in the original. That
makes further look-ups of the same property slower, but it saves memory.

[floitsch, 2014-07-01 09:51:28]

LGTM.

https://codereview.chromium.org/336863008/diff/60001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/60001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/convert_patch.dart:43: else return
_convertJsonToDart(parsed, reviver);
Nit: we generally don't use this kind of "if/else" indentation.

[kasperl, 2014-07-01 09:52:39]

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
File sdk/lib/_internal/lib/convert_patch.dart (right):

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:42: if (reviver == null) return
_convertJsonToDartLazy(parsed);
On 2014/07/01 09:43:33, Lasse Reichstein Nielsen wrote:
> Use braces around the then/else parts.

Done.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:103: if (JS('bool', '# == null',
object)) {
On 2014/07/01 09:43:33, Lasse Reichstein Nielsen wrote:
> Do you need the JS call here, or would
>    if (object == null) return null;
> work as well, because Dart2js treats undefined as null?
> 
> That would also allow the compiler to know that object is not null in the
> following code.

Done.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:107: // JavaScript string, number, bool
are in the correct representation.
On 2014/07/01 09:43:34, Lasse Reichstein Nielsen wrote:
> are in -> already has

Done.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:114: // bug
https://code.google.com/p/v8/issues/detail?id=621 is fixed.
On 2014/07/01 09:43:33, Lasse Reichstein Nielsen wrote:
> According to the issue, the bug was fixed more than a year ago. The d8 in
> third_party has the fix. I think we can start depending on it.

Let's try that as a separate change.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:134: // the map is eventually upgraded
in which case we null it
On 2014/07/01 09:43:34, Lasse Reichstein Nielsen wrote:
> ',' before in?

Done.

https://codereview.chromium.org/336863008/diff/100001/sdk/lib/_internal/lib/c...
sdk/lib/_internal/lib/convert_patch.dart:308: return _setProperty(_processed,
key, result);
On 2014/07/01 09:43:33, Lasse Reichstein Nielsen wrote:
> This sets the property on _processed, even if it doesn't exist in the
original.
> I don't think it's visible, but it will keep the key string alive.
> I think I'd prefer to not store anything if the key is not in the original.
That
> makes further look-ups of the same property slower, but it saves memory.

Done.

[kasperl, 2014-07-01 09:53:34]

Committed patchset #7 manually as r37858 (presubmit successful).