560650 - TM: json code should use JS_Enumerate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

It currently manually sets up an iterator. I can optimize JS_Enumerate for objects and dense arrays (probably not faster yet, but will be faster soon).

Comment 1

[Robert Sayre]

we decided to drop support for __iterator__ because you can now achieve the same effect with the standard ES5 API.

Comment 2

[Robert Sayre]

JS_Enumerate is the right thing. It only grabs own properties.

Comment 3

[Robert Sayre]

Attachment: JS_Enumerate

Comment 4

[Robert Sayre]

Attachment: don't stringify unecessarily

Andreas pointed out that the values from JS_Enumerate are guaranteed to be strings, so I only to stringify when we're pulling values from a whitelist array. He should have minused my first patch, too!

Comment 5

[Andreas Gal :gal]

Comment on attachment 441063 [details] [diff] [review]
don't stringify unecessarily

>+
>+            JSString *ks;
>+            if (JSVAL_IS_STRING(key)) {
>+                ks = JSVAL_TO_STRING(key);
>+            } else {
>+                ks = js_ValueToString(cx, key);
>+                if (!ks)
>+                    return JS_FALSE;
>+            }
>+            AutoValueRooter keyStringRoot(cx, ks);
>+

This is all redundant. Look into js_ValueToStringId. That's exactly what it does internally. Id doesn't need a root either. So just do jsid id; js_ValueToStringId(cx, key, &id).

>+        // We should have a string id by this point. Either from 
>+        // JS_Enumerate's id array, or by converting the values
>+        // in the whitelist.

I think we usually do /*
                       */ style comments but I really don't care.

>+        JS_ASSERT(JSVAL_IS_STRING(ID_TO_VALUE(id)));
> 

r=me with the stuff above fixed. This removes the last use of CallIteratorNext, so if you push this in I can shrink my patch a bit and I already optimized JS_Enumerate() to do a direct memcpy of the ids. Profit!

Comment 6

[Robert Sayre]

(In reply to comment #4)
> Created an attachment (id=441063) [details]
> don't stringify unecessarily
> 
> Andreas pointed out that the values from JS_Enumerate are guaranteed to be
> strings...

...this turns out not to be true. 

JSON.stringify({"2":"foo"});

will get an int id returned from JS_Enumerate. The code can still be simpler than my patch though. Will post a new one.

Comment 7

[Andreas Gal :gal]

Ask discussed via irc and face to face with brendan, JS_Enumerate() used to not give any strong guarantees about the ids, but there are cases in Waldo's ES5 code where it can fail if it returns strings for JSVAL_IS_INT-able ids, so I changed JS_Enumerate() to guaranteee int-ness of JSVAL_IS_INT-able ids. js_EnumerateOwnProperties() is a new internal API which does not intentionally stringify (but it also doesn't guarantee string-ness).

Comment 8

[Brendan Eich [:brendan]]

I made a stronger claim in person, and asked for changes accordingly in the review for bug 558754: JS_Enumerate returns canonical ids. No string ids whose chars form int literals where the int value fits in a JSID_IS_INT jsid.

As with jsvals now (Luke is changing, but jsids will be more exact) we require canonicalization to interned string if not int value, else to int value in jsid (and of course object ids are possible, now with E4X and possibly later with the Harmony Names proposal).

Anything non-canonical is a bug, and we do not error-check for buggy embedders generally -- we assert (recent bugs have been filed to rectify exceptions).

/be

Comment 9

[Robert Sayre]

Attachment: final answer?

this code passes GCZeal, and it's win on my json benchmarks:

TEST              COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:      1.091x as fast    373.9ms +/- 0.2%   342.7ms +/- 0.2%     significant

=============================================================================

  json/stringify: 1.091x as fast    373.9ms +/- 0.2%   342.7ms +/- 0.2%     significant
    test-pattern: 1.129x as fast    210.4ms +/- 0.4%   186.4ms +/- 0.3%     significant
    tinderbox:    1.046x as fast    163.5ms +/- 0.2%   156.3ms +/- 0.2%     significant

Comment 10

[Robert Sayre]

Attachment: forgot to remove some debugging prints from the tests

Comment 11

[Andreas Gal :gal]

Comment on attachment 441279 [details] [diff] [review]
forgot to remove some debugging prints from the tests

Nice. If you land before me I can delete CallIteratorNext when I push.

Comment 12

[Andreas Gal :gal]

And nice speedups by the way ...

Comment 13

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/c9959c6cbfec

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c9959c6cbfec