Closed Bug 612989 Opened 9 years ago Closed 9 years ago

JSON.parse should try to convert numbers to integer

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Whiteboard: [jsperf] fixed-in-tracemonkey)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch PatchSplinter Review
This works and passes tests...
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Attachment #491295 - Flags: review?(sayrer)
With that patch, is there any impact on the performance on number-heavy json parsing?
Attached file JSON number benchmark (obsolete) —
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..
Attached file 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...
Attachment #491326 - Attachment is obsolete: true
This patch has no effect on Kraken's json-parse-financial (tested with -m -j -p)
Attachment #491295 - Flags: review?(sayrer)
Attachment #491295 - Flags: review?(jwalden+bmo)
Attachment #491295 - Flags: review+
Just curious...  how does Opera do on those microbenchmarks?  But in any case, this seems ok from that pov.
(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...
Whiteboard: [jsperf]
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.
It seems like DoubleValue is hazardous, even though we tolerate ints stored as doubles these days. Could it assert without false positives?

/be
There would be false positives; there are several places where, for performance reasons, we use setDouble/DoubleValue over setNumber/NumberValue.
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.
Attachment #491295 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/eea1f443ea15

Good stuff, thanks for the patch!
Whiteboard: [jsperf] → [jsperf] fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b8
http://hg.mozilla.org/mozilla-central/rev/eea1f443ea15
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.