612989 - JSON.parse should try to convert numbers to integer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

The HTML5 Sudoku solver (IE9 test drive, bug 612930) clones boards using this method:

var board = JSON.parse(JSON.stringify(inputBoard));

JSON.parse should try to convert numbers to int32; this will also help other websites and benchmarks.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This works and passes tests...

Comment 2

[Boris Zbarsky [:bzbarsky]]

With that patch, is there any impact on the performance on number-heavy json parsing?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: JSON number benchmark

Good question; this micro-benchmark creates an array of 1000 numbers, converts it to string and then measures the time of parsing it 1000 times.

-m before: 333 ms
-m after: 348 ms
-j before: 346 ms
-j after: 354 ms
v8: 433 ms

So this becomes 8-15 ms slower, but this is a worst-case scenario - parsing 1000 numbers 1000 times without using the result. When you start using the numbers (like the sudoku benchmark) most operations will be faster. 

I will also check kraken's json-parse-financial..

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Number microbenchmark

Fixes a stupid bug in the micro-benchmark (used new Array(1000) and push..)

-m before: 189 ms
-m after: 193 ms
-j before: 190 ms
-j after: 194 ms
v8: 315 ms

So 4 ms slower now, about 2%. I don't know why the difference is smaller now though...

Comment 5

[Jan de Mooij [:jandem]]

This patch has no effect on Kraken's json-parse-financial (tested with -m -j -p)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Just curious...  how does Opera do on those microbenchmarks?  But in any case, this seems ok from that pov.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to comment #6)
> Just curious...  how does Opera do on those microbenchmarks?  But in any case,
> this seems ok from that pov.

For the last test case converted to HTML:
Opera 11: 55 ms
Safari 5: 108 ms
FF4 nightly: 183 ms
Chrome 9: 370 ms

With doubles:
Opera 11: 125 ms
Safari 5: 125 ms
FF4 nightly: 218 ms
Chrome 9: 422 ms

For integers Opera is very fast, for doubles it's as fast as Safari. Safari becomes only slightly slower for doubles vs. integers, I wonder how they're parsing numbers...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fwiw, Opera's json impl is generally really fast...

OK, sounds like the difference in this bug is definitely a blip compared to where we need to be (the difference from Opera's numbers) at least for now.

Comment 9

[Brendan Eich [:brendan]]

It seems like DoubleValue is hazardous, even though we tolerate ints stored as doubles these days. Could it assert without false positives?

/be

Comment 10

[Luke Wagner [:luke]]

There would be false positives; there are several places where, for performance reasons, we use setDouble/DoubleValue over setNumber/NumberValue.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 491295 [details] [diff] [review]
Patch

I agree that the concerns about perf impact in the zillion-numbers case seem unimportant. The way to win there, if it's ever needed, is to not use the slow-and-general parser when parsing a pure integer without exponent/fractional part -- some subtilties, but probably addressable ones.  Maybe that's something to do whenever someone gets around to really optimizing the JSON parser -- but not here, not now, certainly.

Comment 12

[Jeff Walden [:Waldo]]

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

Good stuff, thanks for the patch!

Comment 13

[Robert Sayre]

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