Lars has already drafted an AS3 JSON implementation on Bug 584704. We want a C++ implementation of a JSON encoder/decoder. Why: - It would likely be faster than the AS3 implementation under the current VM. - It would be able to take advantage of features available only to C++ code (such as concurrency in the same vein as Asynchronous XML parsing, as described on Bug 582817).

For a 64-bit DebuggerRelease on my iMac: % avmshell lthJSON.abc testJSONlars.abc run stringify_x500u*100: 355 ms run stringify_x5k*10: 381 ms run stringify_x50k*1: 508 ms run stringify_x500k*1: 5512 ms run parse_y500u: 447 ms run parse_y5k: 474 ms run parse_y50k: 527 ms run parse_y500k: 7342 ms run stringify_c500u*100: 321 ms run stringify_c5k*10: 348 ms run stringify_c50k*1: 397 ms run stringify_c500k*1: 4586 ms run stringify_x500u*1: 142 ms run stringify_c500u*1: 5 ms (Will be posting analogous results for mike chambers and max herkender shortly.)

Again, 64-bit DebuggerRelease on fklockii-iMac: % avmshell mcJSON.abc testJSONmc.abc run stringify_x500u*100: 500 ms run stringify_x5k*10: 570 ms run stringify_x50k*1: 682 ms run stringify_x500k*1: 7672 ms run parse_y500u: 790 ms run parse_y5k: 877 ms run parse_y50k: 851 ms run parse_y500k: 9445 ms This library does not do cycle detection, so all those test cases are thrown out for these runs. (If you're keeping score, that's \inf ms for those tests. :) (Also, to run the Mike Chambers code in the shell, you need to hack the code to replace its import of flash.utils.describeType with something else. But that's not terribly interesting, especially since Lars's code is more performant.)

64-bit DebuggerRelease on fklockii-iMac % avmshell mhEncodeJson.abc mhDecodeJson.abc testJSONmh.abc run stringify_x500u*100: 252 ms run stringify_x5k*10: 268 ms run stringify_x50k*1: 320 ms run stringify_x500k*1: 3527 ms run parse_y500u: 146 ms run parse_y5k: 209 ms run parse_y50k: 184 ms run parse_y500k: 3096 ms Again, no cycle detection in this library, so it loses on all those tests. This library is performing quite impressively compared to Lars's code: between 2x and 3x improvement on the cases it does handle. (It would be interesting to know how much overhead, if any, is being introduced on Lars's code to support cycle detection; but I do not think that is the sole source of performance differential here.) Next task: try out a couple off-the-shelf C/C++ JSON libraries on this same set of tests.

I realized a much easier way (and probably fairer way) to see how a highly optimized implementation would perform is just to port my stress test to Javascript and then try it out on some web browsers. (The hardest part was figuring out how to get the exterior framework into Javascript+HTML, since of course I'm an avmshell hacker, not a web developer. :) Anyway, I cannot guarantee I got all the details right below, but the results for Firefox and Chrome lead me to think that we can probably hit a 10x speed improvement over Max Herkender's highly-tuned ActionJSON code by going with a native implementation. I will illustrate this by including below the speed-up factor for each line when compared against the results F from comment 6 above. Safari 5.0.4: run stringify_x500u*100: 49 ms run stringify_x5k*10: 706 ms run stringify_x50k*1: 9944 ms run stringify_x500k*1: (script timed out) run parse_y500u*100: 20 ms run parse_y5k*10: 20 ms run parse_y50k*1: 24 ms run parse_y500k*1: 300 ms (FYI the above results for Safari may seem crazy, but there are plausible hypotheses for them; Safari may well be generating a lot of intermediate strings in its stringify implementation and then choking on the garbage collections required; that issue does not arise with when parsing.) Firefox 3.6: run stringify_x500u*100: 27 ms 9.3x over ActionJSON run stringify_x5k*10: 33 ms 8.0x over ActionJSON run stringify_x50k*1: 36 ms 8.9x over ActionJSON run stringify_x500k*1: 445 ms 7.9x over ActionJSON run parse_y500u*100: 27 ms 5.4x over ActionJSON run parse_y5k*10: 36 ms 5.8x over ActionJSON run parse_y50k*1: 40 ms 4.6x over ActionJSON run parse_y500k*1: 439 ms 7.1x over ActionJSON Google Chrome 10.0: run stringify_x500u*100: 29 ms 8.7x over ActionJSON run stringify_x5k*10: 33 ms 8.1x over ActionJSON run stringify_x50k*1: 37 ms 8.6x over ActionJSON run stringify_x500k*1: 597 ms 5.9x over ActionJSON run parse_y500u*100: 57 ms 2.6x over ActionJSON run parse_y5k*10: 18 ms 11.6x over ActionJSON run parse_y50k*1: 21 ms 8.8x over ActionJSON run parse_y500k*1: 327 ms 9.5x over ActionJSON Firefox 4.0: run stringify_x500u*100: 16 ms 15.8x over ActionJSON run stringify_x5k*10: 19 ms 14.1x over ActionJSON run stringify_x50k*1: 29 ms 11.0x over ActionJSON run stringify_x500k*1: 402 ms 8.8x over ActionJSON run parse_y500u*100: 19 ms 7.7x over ActionJSON run parse_y5k*10: 23 ms 9.1x over ActionJSON run parse_y50k*1: 30 ms 6.1x over ActionJSON run parse_y500k*1: 611 ms 5.1x over ActionJSON So, its not a 10x improvement across the board, but its enough to make me think that a C/C++ implementation will have plenty of room to stretch its wings.

Comment on attachment 524128 [details] G: simple stress test, html version (heh; using text/html was MIME type turned the attachment into an unintended DOS attack on my Safari browser I use for Bugzilla interactions. Switching to text/plain since the source is what matters unless you're going to run from a file: url anyway.)

Note that the simple stress test provided here (attachments C-G) is focusing heavily on how numeric keys are handled. Its a special case that is probably worth handling well, but the test suite should be expanded to include non-numeric keys. (It is a trivial extension, e.g. just prepend a non-digit to the front of the generated keys.)

fixed a bug and made parsing much more efficient for large sets of uint keys.

Performance of native JSON parse: run parse_y500u*100: 190 ms run parse_y5k*10: 48 ms run parse_y50k*1: 42 ms run parse_y500k*1: 414 ms Notes: * This is from a _very_ naïve direct translation of Lars's AS3 code to C++, literally translating nearly all operations into the corresponding method invocations. There may be much room for improvement; I am just going to simplicity. * The uint key hack referenced in comment 10 was crucial to get parse_y500k down to something competitive with Firefox and Chrome. This means its all the more crucial to extend the stress tests to include a larger variety of keys, in order to ensure I am not optimized one special case but still losing to the competition in general. (It may be interesting to see if the uint key hack is easily back-ported to Lars code, and seeing if there is similar benefit in that case.) * The overhead on parse_y500u is interesting when compared to the other cases below it. Certainly it does not match my predictions from all the other runs one can see from comment 4 through comment 7.

added testcases. So far the acceptance tests are from Lars and ecma3. A test/acceptance/ecma3/JSON/adhoc.as A test/acceptance/ecma3/JSON/e15_12_0.as A test/acceptance/ecma3/JSON/e15_12_1.as A test/acceptance/ecma3/JSON/e15_12_2.as A test/acceptance/ecma3/JSON/e15_12_3.as

Latest work: factored a large portion of stringify to C++. I still need to put in callback support; my side-stepping of the replacer argument does not generalize to the toJSON method. (A callback-free implementation would need to do a prepass to verify that there aren't any toJSON properties set, and that's not really worthwhile until I see evidence that we could get significant speedup from the hypothetical optimizations it would enable). (my lettering system has broken down; maybe I'll try again with A,B,... for patches and M,N,... for test files.)

(In reply to comment #13) > Created attachment 525028 [details] [diff] [review] > H: factor core stringify to C++ > > Latest work: factored a large portion of stringify to C++. > > I still need to put in callback support (oh and its also missing cycle-detection. Again, the only way to avoid doing it on-the-fly is via a prepass to confirm that we won't flow back into user-defined methods; so the conclusion is same as above in comment 13.)

Curiosities from the ECMA-262 spec: 15.12.3 says: "Let K be an internal List of Strings consisting of the names of all the own properties of value whose [[Enumerable]] attribute is true. The ordering of the Strings should be the same as that used by the Object.keys standard built-in function." Section 15.2.3.14 defines the Object.keys function; but all it says about ordering there is "If an implementation defines a specific order of enumeration for the for-in statement, that same enumeration order must be used in step 5 of this algorithm." And finally section 12.6.4 says "The mechanics and order of enumerating the properties (step 6.a in the first algorithm, step 7.a in the second) is not specified." So I guess its up the implementation to decide for itself whether it wants to specify an order. What does this mean for us, and how we should test the ordering of properties in the output?

The ordering of enumeration in AS3 is essentially unpredictable (hash table order), and the first JSON implementation should probably just use that ordering, which it will get either by using for..in enumeration in an AS3 implementation, or by walking the hash table in C++. (We have a job to do on the ordering and enumeration issues, but this month is not the time to do that job.)

(In reply to comment #16) > The ordering of enumeration in AS3 is essentially unpredictable (hash table > order), and the first JSON implementation should probably just use that > ordering, which it will get either by using for..in enumeration in an AS3 > implementation, or by walking the hash table in C++. That matches my inference. dschaffe: I wrote my internal tests with this issue in mind but it looks like it might be an open issue in the tests you uploaded.

New wrinkle: , Dan Schaffer and Lars and I were discussing how properties are handled, and the question of how the fixed properties of AS3 classes should be dealt with. They aren't enumerated by the for/in statement, so a language lawyer might say that skipping them conforms to the spec. But that is unlikely to be the most useful thing to do. Lars suggested I investigate what the existing third-party AS3 JSON libraries do. I will be posting results shortly.

mikechambers library yields: {}: {} new Hello(): {} new StringPair("A", "B"): {"dr":"B","ar":"A"} new Pair("G", "H"): {"dr":"H","ar":"G"} new Pair(1, 2): {"dr":2,"ar":1} new Pair(3, new Pair(4, null)): {"dr":{"dr":null,"ar":4},"ar":3} new PrivatePair(5, 6): {} new InternalPair(7, 8): {} new ProtectedPair(9, 10): {} new GetrPair(11, 12): {"dr":12,"ar":11} new ProtectedGetrPair(13, 14): {} new SetrPair(15, 16): {} maxherkender library (actionjson) yields: new Hello(): {} new StringPair("A", "B"): {} new Pair("G", "H"): {} new Pair(1, 2): {} new Pair(3, new Pair(4, null)): {} new PrivatePair(5, 6): {} new InternalPair(7, 8): {} new ProtectedPair(9, 10): {} new GetrPair(11, 12): {} new ProtectedGetrPair(13, 14): {} new SetrPair(15, 16): {} So: actionjson focuses solely on the dynamic properties, while the mikechambers library handles the fixed properties of AS3 class instances.

(In reply to comment #19) > mikechambers library handles the fixed properties of AS3 class instances. More specifically, it handles the *public* fixed properties of AS3 class instances. (probably a solid choice.) Neither one supports the toJSON method callback. (This is readily verified by reading the library source code.)

(In reply to comment #20) > (In reply to comment #19) > > mikechambers library handles the fixed properties of AS3 class instances. and this statement can be refined to "mikechambers library handles the fixed properties for AS3 class instances, but nothing else." E.g.: (new DynamicPair(17, 18))["gr"]=19: {"ar":17,"dr":18}

Another case of interest: whether fixed properties of one's superclass are included. Here is the result from Mike Chamber's JSON library (it includes the superclass): new SubPair(22, 23): {"ar":22,"dr":23}

Wow, that's almost the definition of "all over the map". Notably the Chambers library handles getters. It's curious (surprising!) that it only handles AS3 fixed properties, but that probably goes to show the class bias of most AS3 programs (AS3 programmers are not like JS programmers after all). IMO including public fixed properties (and even those that come from getters) by default is probably "right" for AS3, including those inherited from base classes of the instance. This widens the scope of the work though. It also causes some complications that we have to think through. For example, what to do with a subclass of Array? Is it serialized as Array? (Probably.)

(In reply to comment #23) > Wow, that's almost the definition of "all over the map". > > Notably the Chambers library handles getters. It's curious (surprising!) that > it only handles AS3 fixed properties, but that probably goes to show the class > bias of most AS3 programs (AS3 programmers are not like JS programmers after > all). I was ambiguous in my wording, and I cannot tell from your response whether I conveyed what was going on. Here's what I should have said: * On objects that are not instances of AS3 classes, Chambers library does the JS thing. * On objects that are instances of AS3 classes, Chambers library traverses the AS3 fixed properties, but *only* the AS3 fixed properties. (This is still curious and surprising, so maybe you did understand me after all.) The test case I think makes the actual behavior clear.

Sweet moses! Hacked in AS3 support! :) :) :) Here's how this handles the analogous test cases to those I posted in attachment 525285 [details] (aka attachment M). (I say analogous because one needs to muck with the test to switch to the JSON.stringify API, etc...) {ar:-1, dr:0}: {"ar":-1,"dr":0} new Hello(): {} new StringPair("A", "B"): {"ar":"A","dr":"B"} new Pair("G", "H"): {"ar":"G","dr":"H"} new Pair(1, 2): {"ar":1,"dr":2} new Pair(3, new Pair(4, null)): {"ar":3,"dr":{"ar":4,"dr":null}} new PrivatePair(5, 6): {} new InternalPair(7, 8): {} new ProtectedPair(9, 10): {} new GetrPair(11, 12): {} new ProtectedGetrPair(13, 14): {} new SetrPair(15, 16): {} new DynamicPair(17, 18)["gr"]=19: {"ar":17,"dr":18,"gr":19} new SubPair(22, 23): {"ar":22,"dr":23}

Just to summarize the state of things, if you compare the output of comment 25 to that of comment 19, the main case that the Chamber library handles "correctly" or "well" that the current C++ patch does not is this line: new GetrPair(11, 12): {"dr":12,"ar":11} vs new GetrPair(11, 12): {} Namely, the Chambers library uses the public getter method to populate those properties. The current C++ patch does not. (I think I'm in a good place now to tackle the issue of callbacks; then I'll be able to handle toJSON, replacer, and if we want, getters, using a common basis.)

Performances results for current patch queue: run stringify_x500u*100: 112 ms run stringify_x5k*10: 121 ms run stringify_x50k*1: 142 ms run stringify_x500k*1: 1517 ms run parse_y500u*100: 196 ms run parse_y5k*10: 49 ms run parse_y50k*1: 46 ms run parse_y500k*1: 510 ms run stringify_c500u*100: 113 ms run stringify_c5k*10: 141 ms run stringify_c50k*1: 98 ms run stringify_c500k*1: 1125 ms run stringify_x500u*1: 158 ms run stringify_c500u*1: 3 ms check x500u y500u check x5k y5k check x50k y50k check x500k y500k check ! x5k y50k check ! x50k y500k check ! x500k y5k

(In reply to comment #27) > Performances results for current patch queue: Crud, this is different hardware (my laptop), so the results in comment are not comparable to the results from earlier in the ticket.

Heard through grapevine: JSON tests (stress tests?) available in Kraken benchmark: http://krakenbenchmark.mozilla.org http://hg.mozilla.org/projects/kraken/file/tip/tests/kraken-1.1

Table of results gathered for 64-bit ReleaseDebugger on my MacBookPro. (The C++ patches are the Klock entry that runs down the center below.) Average of Time for 3 iterations (ms) Cham~ Hans~ Herk~ Klock Chrome FF3.6 FF4.0 ----- ----- ----- ----- ------ ----- ----- parse_y500k*1 12244 9316 4777 573 376 616 716 parse_y500u*100 1010 747 163 152 19 38 24 parse_y5k*10 1089 592 329 49 54 41 29 parse_y50k*1 1095 647 312 48 46 50 41 strfy_x500k*1 10055 7374 4311 1541 974 622 574 strfy_x500u*100 609 500 283 155 49 33 19 strfy_x50k*1 849 585 404 155 56 60 44 strfy_x5k*10 727 490 314 128 63 49 30

Sigh. Fixed presentation for Comment 30: Average of Time for 3 iterations (ms) Cham~ Hans~ Herk~ Klock Chrome FF3.6 FF4.0 parse_y500k*1 12244 9316 4777 573 376 616 716 parse_y500u*100 1010 747 163 152 19 38 24 parse_y5k*10 1089 592 329 49 54 41 29 parse_y50k*1 1095 647 312 48 46 50 41 strfy_x500k*1 10055 7374 4311 1541 974 622 574 strfy_x500u*100 609 500 283 155 49 33 19 strfy_x50k*1 849 585 404 155 56 60 44 strfy_x5k*10 727 490 314 128 63 49 30

revised approach to stringification. still not as fast as it could be. And it no longer corresponds to Lars's AS3 code as closely. but more performant than before (hopefully to a noticeable degree).

Comment on attachment 526093 [details] [diff] [review] error strings for json (tommy R+'ed in #tamarin.)

better than before. - improved stringify performance. - skip AS3 fields marked as [Transient]. - added support for when replacer is Array (ie user-provided property list) - fixed various output bugs - a little bit of code clean up but much remains still

spent weekend speeding this up (and fixing bugs). Posting for another handoff to dschaffe for testing.

minor update; i left the manifest.mk changes out of attachment 526702 [details] [diff] [review].

Comment on attachment 524491 [details] [diff] [review] acceptance tests Obsoleted by BUg 649800.

Checkpointing work as of evening of April 18th. Will be posting review requests for this.

Comment on attachment 526892 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. I'm going to post review requests to Tom, Rick, and Ruchi. Its meant to be interpreted as an "OR", not an "AND" -- that is: "if you feel comfortable doing the review, and can get to it ASAP, please take it, and remove the review requests from the others." Also, if you don't feel comfortable doing this review but can suggest someone else who would be appropriate who is not on PTO, I am all ears!

Comment on attachment 526892 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. See comment 40.

Comment on attachment 526892 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. See comment 40.

(We want the review ASAP because this had wanted this to land last Friday.)

(minor cleanup and a big bugfix for Vector<> handling.) Same story as before: I'm requesting several reviewers, but I only want/need one person to claim it and let the other people off the hook, please. (And again, if you don't want to do it but have a suggestion for someone else, I'm all ears.)

Comment on attachment 526906 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. (see comment 40 and comment 44)

Comment on attachment 526906 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. (see comment 40 and comment 44)

I wanted to double-check my understanding of ECMA-262, so I whipped up this Javascript exploration. It goes through phases, overriding the toString() and toJSON methods of Number.prototype and String.prototype, and presents the results (both for direct literals and for instances of new Number() and new String(), since that is a source of semantic distinction in ECMAscript, unlike ActionScript). The results are interesting; FireFox and Chrome disagree in one case, it seems. I think FireFox is matching ECMA-262. Anyway the main take-away for me is that I don't need to worry further about toString(); my behavior is matching FireFox (I think), and if someone wants to override how Numbers and Strings print in JSON, they should be able to override the toJSON() method.

(small change to presentation to clarify what is being tested)

this is part of the JSON work. An important goal is that clients can override toJSON in the prototypes for various classes. We cannot provide sane default toJSON to ByteArray or Dictionary, so the onus is on the client to do whatever their application might require. Please sanity check.

(In reply to comment #44) > Created attachment 526906 [details] [diff] [review] > patchA': json support spread across AS3 and C++ code. > > (minor cleanup and a big bugfix for Vector<> handling.) > > Same story as before: I'm requesting several reviewers, but I only want/need > one person to claim it and let the other people off the hook, please. (And > again, if you don't want to do it but have a suggestion for someone else, I'm > all ears.) Note to reviewers: I am aware of one detail missing from the current implementation: when no replacer array is provided, the code does not make a copy K of the keys before it starts its enumeration. (See Lars' code for an example of how this could be done.) I do plan to post another update with such a change. But that will be a relatively small change to the code (and such a change could be classified as a bug-fix rather than a feature-addition), which is why I decided to post review requests on the patch as of last night, just to get the ball rolling.

Comment on attachment 527022 [details] [diff] [review] Various AS3 classes need toJSON I wonder if the lack of compatibility with Date.toJSON might create some interop issues with JS. We could try to match the behavior of toISOString here, but I haven't checked how toString and toISOString might diverge, so it may not be a big deal.

(In reply to comment #51) > Comment on attachment 527022 [details] [diff] [review] > Various AS3 classes need toJSON > > I wonder if the lack of compatibility with Date.toJSON might create some > interop issues with JS. We could try to match the behavior of toISOString here, > but I haven't checked how toString and toISOString might diverge, so it may not > be a big deal. I considered that, but it seemed like an important property would be to ensure that one can parse the generated output back into a Date object from ActionScript. From my skimming of the Date class, it did not look like we would parse the ISO format in the Date constructor, nor did I see other methods to parse that format. Am I incorrect about that?

There is not support for the ISO 8160 date in ActionScript. ISO date support was added in ES5.

(this patch is meant to be applied on top patchA', ie attachment 526906 [details] [diff] [review] as of the moment.) Added code to handle scenario where no property list was provided; in this case, you need to build up intermediate array of each object's own property names before traversing the object. ECMA-262's spec for JSON says you need to do this. Lars was careful to include it. My understanding is that it is meant to catch cases where the toJSON() or replacer() methods muck with the object's internal properties. As part of this, I ended up factoring out the portions that call out to toJSON() and the replacer() method and put them into their own separate methods, mostly because it was the easiest way to deal with warnings about state being clobbered by the potential longjmp performed by the call out.

Comment on attachment 526906 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. adding bill to list of reviewers. Bill: this list of reviewers is meant to be interpreted as an OR. If you have started the review and believe you will finish it in the near future, please grab the conceptual lock and remove the other reviewers from the request list. Thanks!

I have not reviewed all the files but here are few review comments for the files I already looked: 1. I see indentation issues in JSON.as 2. In JSONClass declaration in JSONClass.h: you need to keep GC_NO_DATA(JSONClass) in public access section. Currently you want DECLARE_SLOTS_JSONClass; as private but due to the macro GC_DECLARE_EXACT_METHODS it becomes public. [See GC_DECLARE_EXACT_METHODS in GC.h]

I am doing a review, but I will not discourage Ruchi from being another set of eyes.

Patches A' and B' look reasonable, except for one overt bug noted below. There are some code quality issues however that should be resolved. On the whole, the code was just harder to understand, as a reader progressing through the code in more or less sequential fashion, than it should have been. There appears to be a lot of room for polishing the code with more careful, descriptive naming and restructuring for clarity. More/better header comments and general architecture/overview commentary would have helped, and some of my specific remarks might be revised or retracted if the intent were more clearly conveyed up-front. None of this, however, rises to the level that would warrant a significant delay in getting this feature out the door. I'd be content to let it go with the proviso we follow up promptly with a patch to shave off the stylistic rough edges. Before I declare the review finished, I'd like to revisit a few points with ECMA 262 in hand, but I've attached my comments following an end-to-end walkthrough here. BUG: default: // control character => \u#### len = sizeof(int_conv_buf); ptr = MathUtils:: convertIntegerToStringBuffer(c+0x10000, (char*)int_conv_buf, buflen, 16, MathUtils::kTreatAsUnsigned); In JSONSerializer::Quote(), the argument 'buflen' to convertIntegerToStringBuffer() is never initialized. It appears that we meant to initialize it with the previous line, but in fact clobbered the live variable 'len', which is the length of the original string we are quoting and emitting.

(In reply to comment #58) > In JSONSerializer::Quote(), the argument 'buflen' to > convertIntegerToStringBuffer() is never initialized. It appears that we meant > to initialize it with the previous line, but in fact clobbered the live > variable 'len', which is the length of the original string we are quoting and > emitting. holy cow, that's a big one. am making regression test now.

(I agree with the code quality comment from Bill. I will work on that today after I fix the bug he noted and add in checks for stack exhaustion.)

(In reply to comment #58) > Created attachment 527207 [details] > Extended review comments on patches A' and B' Bill: Thanks for the very thorough review! I know it was a last minute request, and it is much appreciated! I will soon work on incorporating your feedback (as well as fix some other problems that have cropped up). You should probably not start a second round of reviewing (the one you wanted to do with ECMA-262 in hand) without pinging me first, because the patch you have will almost certainly be out of date very soon. One item I will note immediately: You comment: It would be faster to invoke the reviver in C++ code, perhaps as each object is read. I presume we aren't particularly concerned with the performance of revivers. Are there important performance-critical use cases? My understanding is that we *cannot* invoke the reviver as each object is read. I am basing this in part on the following comment from Lars's original AS3 code: // Observe that it is not possible to intermingled the walker with the parser, both // because the walker is provided the 'holder' object as its 'this' value and will // expect to see that fully populated, and because it would result in re-visiting // objects (and make the cost of walking exponential.) It is possible that I misunderstand the situation. In any case, for now I am assuming that performance of revivers is not important.

Fixed various bugs, including the big one that Bill noticed. Also fixed things up so that the patch gets through the sandbox builder. I do need to still incorporate the remainder of Bill's feedback. Will start on that now.

This is identical to attachment 527293 [details] [diff] [review] except its stringify has a stack limit check. JSON.parse still needs one, though. (I'd be curious to know if Chrome and/or Firefox can handle an object with 1e5 levels of nesting.)

Added stack limit check to JSON.parse, as well as a comment justifying why the machinery used in JSON.stringify to unroll the stack is not necessary here. (Maybe I should have used GC-managed memory for my ropes and just punted on the whole ReturnCondition structure. But that's too large a refactoring for me to take on right now.)

Comment on attachment 527311 [details] [diff] [review] patchA': json support spread across AS3 and C++ code. obsoleted by attachment 527327 [details] [diff] [review]

(In reply to comment #61) > (In reply to comment #58) > > Created attachment 527207 [details] > > Extended review comments on patches A' and B' > > Bill: Thanks for the very thorough review! I know it was a last minute > request, and it is much appreciated! > > I will soon work on incorporating your feedback (as well as fix some other > problems that have cropped up). You should probably not start a second round > of reviewing (the one you wanted to do with ECMA-262 in hand) without pinging > me first, because the patch you have will almost certainly be out of date very > soon. Bill: Putting in stack limit checks and testing them took me longer than I thought, so I don't think I'm going to get to the clean-up items as soon as I had hoped. Feel free to start a second round of review on attachment 527327 [details] [diff] [review] without pinging me first, I have to focus on broader integration work now, unfortunately.

(now that both parse and stringify can potentially throw AS3 exceptions while in the midst of their work, due to the addition of stack-limit checks, the architecture of manually unrolling the stack by passing up a ReturnCondition token seems completely bonkers. But I think I need to land as-is today, and make removing the ReturnCondition business a followup work-item.)

(In reply to comment #56) > I have not reviewed all the files but here are few review comments for the > files I already looked: > > 1. I see indentation issues in JSON.as Yes, those definitely need resolution. That will be part of my cleanup pass that I promised Bill. > 2. In JSONClass declaration in JSONClass.h: you need to keep > GC_NO_DATA(JSONClass) in public access section. Currently you want > DECLARE_SLOTS_JSONClass; as private but due to the macro > GC_DECLARE_EXACT_METHODS it becomes public. [See GC_DECLARE_EXACT_METHODS in > GC.h] Ruchi and I discussed this in the chat room. I don't need to make any changes, but Ruch is right that the current code is misleading because the GC_NO_DATA macro implicitly introduces and ends with a public: modifier. (Maybe GC_DECLARE_EXACT_METHODS should end with "private:" to force programmers to choose an alternative when necessary, rather than letting the implicit public: leak through. Or maybe that is a ridiculous idea.)

(In reply to comment #66) Comments on changes from previously-reviewed patch: JSONClass.cpp ------------- Rope probably warrants being left as an encapsulated class, and declared at top-level. AutoDestructingAtomArray is fine as a struct. Likewise Chunk, which is appropriately nested within Rope. > char *concatenated = concat(); > int32_t len = length(); > String *str = core->newStringUTF8(concatenated, len); You go to some trouble to use utf8_t* for the buffer, but when we get to toStringViaConcat(), the concatenated string is a char*. Note that newStringUTF8() is declared to take a char* argument. We appear to assume that sizeof(utf8_t) == sizeof(char) and sizeof(utf16_t) == sizeof(wchar) in AvmCore. According to RFC 4627, JSON text may be encoded in UTF-8, UTF-16, and UTF-32 (with UTF-8 the default). I don't see any code to recognize the encoding in the parser. Have we previously converted the string to UTF-8? Do we have test cases with JSON input containing non-Latin1 characters? > wchar JSONParser::hexDigitValue(wchar ch) I would call this 'extendedDigitValue' if 0-9A-Za-z is allowed, otherwise restrict to 0-9A-Fa-f . > - Quote(core()->uintToString(key)); > + Quote(core()->uintToString(uint32_t(atomGetIntptr(key)))); Oops! I shouldn't have missed that one before. Good catch!

By design, JSON.stringify() differs from ECMA-262 in throwing an exception for an invalid replacer argument. In contrast, JSON.parse() silently ignores a non-null non-callable reviver parameter until a call is attempted, whereupon we presumably throw a non-JSON-specific exception. Would it be appropriate to check the parameter earlier? ECMA-262 doesn't even attempt the walk unless the reviver is callable. Should we follow suit? Either way, we are not consistently following ECMA-262 or consistently doing aggressive validation.

(In reply to comment #70) > By design, JSON.stringify() differs from ECMA-262 in throwing an exception for > an invalid replacer argument. In contrast, JSON.parse() silently ignores a > non-null non-callable reviver parameter until a call is attempted, whereupon we > presumably throw a non-JSON-specific exception. Would it be appropriate to > check the parameter earlier? ECMA-262 doesn't even attempt the walk unless the > reviver is callable. Should we follow suit? Either way, we are not > consistently following ECMA-262 or consistently doing aggressive validation. Filed as Bug 651793.

Bill's comments, my further refinement of how I handle AS3 exceptions (mainly for stack limit checking), and the need to fix -Dinterp exception handling, led me to throw out the whole fail_{atom,ptr,etc} debacle and go straight to using exceptions for invalid input, which is the way it should have been from the start. I'm still not 100% clear on why -Dinterp was exposing a problem before, though. If I had more time I would have cleared that question up, but for now this is a strict improvement on what came before (in many ways).

This should address Bug 651793. (I know, it seems crazy that I keep putting bugfixes here but file them in separate tickets. I just want everything in one place so that I can grab it all easily; its just some insanity for today alone.)

patchB' (attachment 527580 [details] [diff] [review]) looks good, and is much cleaner than before. patchC' (attachment 527585 [details] [diff] [review]) looks good.

(In reply to comment #73) > Created attachment 527585 [details] [diff] [review] > patchC': check that reviver is valid in glue code > > This should address Bug 651793. > > (I know, it seems crazy that I keep putting bugfixes here but file them in > separate tickets. I just want everything in one place so that I can grab it > all easily; its just some insanity for today alone.) patchC' might be unnecessary; I am in the process of trying to figure out how the AS3 type annotations fit into all this. (When I attempt to pass in a non-Function as the second parameter to parse, I get an error before control even reaches the innards of parse. That's why I'm having a little bit of confusion.) We needed the explicit checks in stringify because the replacer argument there is of type '*', since it can be either an Array or a Function. (Ugh, I know; come talk to me some time about whether that conflation actually makes sense, or if it introduces interesting expressiveness issues for the two methods of invoking JSON.stringify.)

(In reply to comment #75) > (In reply to comment #73) > > Created attachment 527585 [details] [diff] [review] > > patchC': check that reviver is valid in glue code > (When I attempt to pass in a non-Function as the second parameter to parse, I > get an error before control even reaches the innards of parse. That's why I'm > having a little bit of confusion.) That's because the argument is declared statically as Function, and a non-callable object cannot be coerced to one. > We needed the explicit checks in stringify because the replacer argument there > is of type '*', since it can be either an Array or a Function. Ah. So possibly we don't even want an exception for the reviver -- we have a perfectly good static type system that ECMAscript lacks. This is an API design issue that should be treated as such, but it seems reasonable to leave it this way.

(In reply to comment #76) > (In reply to comment #75) > > We needed the explicit checks in stringify because the replacer argument there > > is of type '*', since it can be either an Array or a Function. > > Ah. So possibly we don't even want an exception for the reviver -- we have a > perfectly good static type system that ECMAscript lacks. This is an API design > issue that should be treated as such, but it seems reasonable to leave it this > way. I don't know if its going to cause more harm than good to remove the entry from the error constants table at this point. I suppose I could leave the error id allocated, but remove the text; that way the localization team might not waste time on it.

This should fix Bug 651641. (Including the ByteArray issue that scope-crept in.)

(In reply to comment #52) > (In reply to comment #51) > > Comment on attachment 527022 [details] [diff] [review] > > Various AS3 classes need toJSON > > > > I wonder if the lack of compatibility with Date.toJSON might create some > > interop issues with JS. We could try to match the behavior of toISOString here, > > but I haven't checked how toString and toISOString might diverge, so it may not > > be a big deal. > > I considered that, but it seemed like an important property would be to ensure > that one can parse the generated output back into a Date object from > ActionScript. > > From my skimming of the Date class, it did not look like we would parse the ISO > format in the Date constructor, nor did I see other methods to parse that > format. > > Am I incorrect about that? (In reply to comment #53) > There is not support for the ISO 8160 date in ActionScript. ISO date support > was added in ES5. See in particular Bug 609853

oh wait, there was still a bit more (small) cleanup and a super-review to request. I'll file a cleanup bug.

(In reply to comment #80) > oh wait, there was still a bit more (small) cleanup and a super-review to > request. I'll file a cleanup bug. Filed as 653255

(In reply to comment #81) > (In reply to comment #80) > > oh wait, there was still a bit more (small) cleanup and a super-review to > > request. I'll file a cleanup bug. > > Filed as 653255 ugh Bug 653255