JSON3 hangs IE7 with particular json data with long string property values.

[jaxley]

I created a jsperf to demonstrate the issue. Even in IE8 the issue is pronounced (allowing a mere 6 ops/second). http://jsperf.com/json-parser-perf-ie-largestringproperties

We experienced an issue where json3 was causing the IE7 browser to literally hang. I've tried running the jsperf in IE7 and it has only hung the browser thus far so there is no data yet. IE8 took a while but it did finish.

Without IE profiler, I can see if I can generate some metrics from dynatrace to identify which specific function(s) or aspects of the functions might be the culprits.

EDIT: Dynatrace shows only one stand-out function and that is the lex() function when processing the string property value itself. It took 4779ms to parse that one string. I'm not sure why sometimes the browser hangs in our real environment (or the jsperf) but it doesn't hang when running the same parse in isolation, other than for the 5 seconds or more it's parsing and the cpu is pegged.

<?xml version="1.0" encoding="utf-8"?><browser_purepath_tree><nodeinfo level="7" function="lex()" arguments="" return="@&lt;div class=&quot;columns columns-aligned ...TRUNCATED for brevity" start_ms="7658.334229629743" total_ms="4778.469051234164" exec_ms="4778.46923828125" exec="38.39862" api="json3, JavaScript" agent="Browser"/></browser_purepath_tree>

[jaxley]

Not sure if I'll have more cycles to play with this in the near-term, but looking at the code, I see += used to build up the return string during parsing. Older IE is known to have terrible string performance, especially for very large strings (can be geometric or exponential falloff in performance) so I suspect that an alternative polyfill for IE that uses Array.join may substantially increase performance over +=. I'll see if I can do a quick comparison test and report back.

[jaxley]

Changing the += did not have a significant performance improvement. But changing how the string is iterated over did have a dramatic impact. Down to 936ms from almost 5 seconds -- almost 4 seconds faster.

In IE7, it is faster on big strings to convert the string into an array of characters and then traverse the array by index.

in JSON.parse, split the string into an array right away for use by the multiple internal invocations of lex():

Source = source;
S2 = Source.split('');

Then change everywhere in lex() to use S2[Index] instead of source.charAt(Index)

<?xml version="1.0" encoding="utf-8"?><browser_purepath_tree><nodeinfo level="7" function="lex()" arguments="" return="@&lt;div class=&quot;columns columns-aligned ...TRUNCATED for brevity" start_ms="7591.719617996143" total_ms="936.1890966589308" exec_ms="936.1890869140625" exec="10.614694" api="json3, JavaScript" agent="QA5Browser-o049539@cigdomardev:221240"/></browser_purepath_tree>

Would need to compare this across other browsers as it may perform worse in non-IE browsers. That would determine whether you would want to abstract the indexing to allow for browser-specific polyfills.

Another thing I noticed in the code is the prefixed increment, e.g. source.charAt(++Index). But javascript only supports postfix increment so is often better to write this as source.charAt(Index++) or even better source.charAt(Index+=1) to denote what javascript is actually doing and avoid confusion.

[ghost]

Wow, that's quite a staggering difference. Thank you so much for taking the time to run the profiler and report the results. I suspect that, for other browsers, it'll be faster just to use [] directly on the string (IE <= 7 doesn't support accessing characters this way; IE 8 only supports it for string primitives, not object wrappers). For those versions, I'll follow your suggestion to split the string into an array of characters before parsing.

Thanks again! I'll find some time to make the patch, run benchmarks, and cut a release in the next few days.

[jeffrose]

+1

[jdalton]

👯

[jeffrose]

Any update on the status of this issue?

[whawker]

Will this be merged to master? Just been massively stung by this bug. Had to revert back to JSON2 in the meantime.

[ghost]

Sure thing—I'm just finishing up testing and benchmarking v3.2.5. Aiming for a Friday release.

[rcollette]

Can this be closed since you are at 3.3 already?

[jaxley]

I'm not on this project anymore. I suspect it would be fine given ie7 is
not very relevant anymore.
On Mar 24, 2014 7:12 AM, "Richard Collette" notifications@github.com
wrote:

Can this be closed since you are at 3.3 already?

Reply to this email directly or view it on GitHubhttps://github.com//issues/23#issuecomment-38448643
.

[bnjmnt4n]

I won't be closing this for now, as the issue still exists. See http://jsperf.com/json-parser-perf-ie-largestringproperties/7. I intend to do some more benchmarking to optimize this first.

[bnjmnt4n]

I am completely lost on how to improve performance for IE. Any ideas, @kitcambridge? Also /cc @jdalton

[ghost]

@D10 I've been pondering removing the recursive descent parser, and using a character-based (rather than RegExp-based) validation step followed by Function/eval. Purely speculative at this point, though; I haven't had time to break out the validator or run any benchmarks.

[bnjmnt4n]

@jdalton Any ideas for improving performance of JSON.parse, especially the strong string iteration portion?

[bnjmnt4n]

@kitcambridge if it’s possible could you try to optimize our parse and stringify implementations? They both seem much slower than JSON 2’s: http://jsperf.com/json-parser-perf-ie-largestringproperties/16 and http://jsperf.com/json3-tests/2.

[ghost]

Wow, that's quite a drop from v3.3.2. I'll profile and see what's going on; maybe this weekend or next week. I doubt we'll ever match the performance of JSON 2's parse (especially in older environments) without rethinking our parser. I would guess stringify is hampered by the cyclic check and forOwn call, but that's purely speculative without a profile.

[oxyc]

The main issue is commit cefa14a

-          Source = "" + source;
+          Source = new String(source);

v8 bailsout on charCodeAt as it's an object instead of a string. See code-stubs.h.

[oxyc]

I ran a quick proof-of-concept with simply validating the json with the current recursive descent parser, and then using eval. Speed does improve significantly on newer browsers. Unfortunately IE9 turned out to be slower, and I do not have access to IE7-8, so it requires more research. JSON3 latest with the fix I pointed out earlier is roughly on par with JSON3 v3.3.2.

http://jsperf.com/json-parser-perf-ie-largestringproperties/18

The JSON3_EVAL is the latest version with the patch https://gist.github.com/oxyc/6d02b15b745638805afd (all tests pass)

[bnjmnt4n]

@oxyc Thanks for the research, I'll remove the new String usage. Will experiment with the others. Here's the latest jsPerf: http://jsperf.com/json-parser-perf-ie-largestringproperties/19.

[oxyc]

For reference, I was curious to what it would look like if string literals were validated with a regex instead. This increased the speed in chrome (past nativeJSON for this particular edge case) but decreased in IE9 once again.

I basically replaced the string literal loop in lex() with a naive regex (fails some tests and is not valid in the more common cases... but works well enough for benchmarking to see if it's even worth looking into).

                var reNeedsEscape = /"([^"\\]*|\\["\\bfnrt\/]|\\u[0-9a-fA-F]{4})*"/;
                var reEscape = RegExp(reNeedsEscape.source, "g");
                var res = reEscape.exec(source.slice(Index));
                if (res) Index += res[0].length;
                else abort();
                return "@"

In case you want to check IE8 here's the jsPerf: http://jsperf.com/json-parser-perf-ie-largestringproperties/20 (includes the eval parsing modification from earlier).

[bnjmnt4n]

I think that it’s not very important to have better performance for modern browsers (Chrome) as they would already have valid JSON.parse implementations. We should instead focus on older browsers (IE 6 - 8, etc.).

[oxyc]

I do agree, I'm mostly trying to come up with ideas that could work. Unfortunately I deleted my VMs some time ago and now I do not have access to IE8 more.

The latest hack seemed like quite a good push for IE8 (186 ops for JSON3 vs 286 ops for JSON2). But I'm surprised of the how big the difference still is, considering how small the loop in lex() is (33 iterations).

[bnjmnt4n]

Yep, I’m also experimenting, currently with a non-recursive implementation. Also, if you need access to IE 8, you could use an online service like Sauce Labs.

[bnjmnt4n]

Here’s my non-recursive implementation: https://github.com/d10/json3/commit/507278b9f9684b608bec3a2f2a626a9d44797a87. I haven’t tested it yet, will try to do so soon.

[ghost]

@D10 Nice! Another approach might be to use a validating parser (here's a very rough, untested port of Go's encoding/json state machine), followed by new Function('return (' + source + )'). That has its own drawbacks, though. I'll run some benchmarks once I have access to my VMs again.

[bnjmnt4n]

Here’s a jsPerf: http://jsperf.com/json-parser-perf-ie-largestringproperties/23. Seems much slower than current. @oxyc’s regex implementation seems to be the fastest. I will also try to experiment with my non recursive implementation, which currently dies not work.

[ghost]

Cool; RegExp it is. We can tighten it to perform additional validation and measure its impact. Thanks, @oxyc!