sile / jsone

Erlang JSON library
MIT License
291 stars 71 forks source link

Reduce floating point errors in decoding #66

Closed kusano closed 2 years ago

kusano commented 2 years ago

Before:

1> Json = jsone:encode(123.0).
<<"1.23000000000000000000e+02">>
2> jsone:decode(Json).
123.00000000000001

After:

1> Json = jsone:encode(123.0).
<<"1.23000000000000000000e+02">>
2> jsone:decode(Json).
123.0

Although floating point numbers may have errors, numbers which can be represented without errors should be treated without errors. This patch reduce floating point errors by avoiding the use of decimals.

1> 123000000000000000000.0 * 1.0e-18.
123.00000000000001
2> 123000000000000000000.0 / 1.0e+18.
123.0

In spite of that 123000000000000000000 > 253, 123000000000000000000 can be exactly represented in IEEE 754 format since it can be divided by 2 many times.

My concern is performance. Division may be slower than multiplication.

My coworker wrote this patch and he has agreed to post it here.

sile commented 2 years ago

Thanks for your PR! LGTM.

For your performance concern, I tried running a simple benchmark as follows:

% Common Setup
> Input = [123.0 || X<-lists:seq(1, 100000)].
> Json = jsone:encode(Input).

% `master` branch
> timer:tc(fun jsone:decode/1, [Json]).
{66039, % 66ms
 [123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001,
  123.00000000000001,123.00000000000001,123.00000000000001|...]}

% PR branch
18> timer:tc(fun jsone:decode/1, [Json]).
{65655, % 65ms
 [123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0,
  123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0,
  123.0,123.0,123.0,123.0,123.0,123.0,123.0,123.0|...]}

There are no significant performance changes. So I think it's okay to merge this PR.

sile commented 2 years ago

Note that I'll release the next version including this patch within this month.