Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct exponential representation for rational values #493

Merged
merged 7 commits into from
Jul 16, 2020

Conversation

Tropid
Copy link
Contributor

@Tropid Tropid commented Jun 14, 2020

This Pull Request addresses #413

It changes the following:

  • Uses Rust exponential display for numbers which should be represented in scientific notation
  • Adds a few test cases

This is a pretty simple way to deal with the problem at hand. It just uses Rusts exponential formatting for values that require the scientific notation. It has a few problems, though:

  • I'm not sure if the boundary conditions work correctly in every circumstance (probably not)
  • It has to parse the string to replace the default output of the rust exponential formatting (.replace("e", "+e"))

I looked at various documents regarding converting doubles to decimal representation. There is V8s implementation (as linked in the Issue) which seems to use different algorithms based on the number (see DoubleToAscii() here: https://chromium.googlesource.com/v8/v8/+/refs/heads/master/src/numbers/dtoa.cc). The other algorithm is called BignumDtoa in V8.
Rust itself uses a combination of the Grisu and the Dragon families of algorithms (see https://github.com/rust-lang/rust/blob/master/src/libcore/num/flt2dec/mod.rs#L49).

Also there seems to be a new algorithm which may yield better results:

I see three options to continue this issue:

  1. Use this and maybe replace it later on to squeeze out more performance

This is a rather simple/easy to understand solution. It has a rather advanced implementation (Rust std/core library). It needs not much maintenance as it uses the std library. It's probably good enough for the first time/most use cases.

Also we can later replace it with more advanced algorithms.

  1. Implement BignumDtoa from V8. This seems to be the general purpose function to format any double (V8 tries faster methods before falling back to this one).

  2. (Re-)Implement the full amount of different algorithms

This seems rather complicated (at least to me) and difficult to maintain/tweak the performance to reach at least the same as the std implementation.

  1. ?

I'd be glad to hear some more input on this topic.

@codecov
Copy link

codecov bot commented Jun 14, 2020

Codecov Report

Merging #493 into master will increase coverage by 1.62%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   68.19%   69.81%   +1.62%     
==========================================
  Files         172      172              
  Lines       10573    10757     +184     
==========================================
+ Hits         7210     7510     +300     
+ Misses       3363     3247     -116     
Impacted Files Coverage Δ
boa/src/builtins/value/display.rs 80.64% <100.00%> (+2.18%) ⬆️
boa/src/builtins/value/tests.rs 100.00% <100.00%> (ø)
boa/src/exec/tests.rs 100.00% <0.00%> (ø)
boa/src/builtins/bigint/tests.rs 100.00% <0.00%> (ø)
boa/src/builtins/number/tests.rs 100.00% <0.00%> (ø)
boa/src/builtins/object/tests.rs 100.00% <0.00%> (ø)
boa/src/syntax/lexer/mod.rs 56.09% <0.00%> (+0.24%) ⬆️
boa/src/builtins/value/operations.rs 30.87% <0.00%> (+0.46%) ⬆️
boa/src/builtins/object/internal_methods.rs 30.36% <0.00%> (+0.52%) ⬆️
boa/src/builtins/function/mod.rs 69.42% <0.00%> (+0.63%) ⬆️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b3d52b...1eb0db0. Read the comment docs.

@HalidOdat HalidOdat added the enhancement New feature or request label Jun 14, 2020
@HalidOdat HalidOdat added this to the v0.9.0 milestone Jun 14, 2020
@HalidOdat
Copy link
Member

HalidOdat commented Jun 14, 2020

Hmmm. I'm fine with having this for the time being, what we had before was not spec compliant at all. And in the future we can improve it.

I think we should add more tests for this to see how spec compliant it is.

What do you think? @Razican @jasonwilliams

@Tropid
Copy link
Contributor Author

Tropid commented Jun 14, 2020

This Ryu thing looks really interesting, though.

It looks like the original author of Grisu3 (which seems to be used for fast conversion in V8) even said Ryu is faster than Grisu3:
https://www.hackernext.com/story?id=20181832

I'd really be interested in implementing this algorithm.

EDIT:

There is already a crate: https://crates.io/crates/ryu-ecmascript
This suggests that this algorithm fulfills ECMAScript's requirements (https://tc39.es/ecma262/#sec-numeric-types-number-tostring).

EDIT2:
See also: ulfjack/ryu#87

@croraf
Copy link
Contributor

croraf commented Jun 18, 2020

Excelent research from @Tropid. I think this is important. The stringification of numbers must be compliant with https://tc39.es/ecma262/#sec-numeric-types-number-tostring. This will likely also solve #449.

How much work do you think you need @Tropid ?

@Tropid
Copy link
Contributor Author

Tropid commented Jun 21, 2020

@croraf sorry, I forgot to reply after we talked about it in the Discord chat.

I just updated the pull request. It now uses the ryu algorithm to print the numbers.

I forked the dtolnay/ryu project/crate and adjusted it for ECMAScript formatting. This draft PR adds a dependency to my fork. I'd suggest for Boa to fork this project itself and cherry-pick my changes or something like that, so that the repository is accessible by Boa and can be updated regularly.

@HalidOdat What are your opinions on that?

@HalidOdat
Copy link
Member

HalidOdat commented Jun 21, 2020

@HalidOdat What are your opinions on that?

Good job! @Tropid.

I agree, we may also want to publish this to crates.io, since ryu-ecmascript looks to be unmaintained, has not been updated in 2 years.

What do you think @Razican @jasonwilliams

@jasonwilliams jasonwilliams modified the milestones: v0.9.0, v0.10.0 Jun 25, 2020
@Razican
Copy link
Member

Razican commented Jul 8, 2020

What do you think @Razican @jasonwilliams

Let's do this :)

@HalidOdat HalidOdat marked this pull request as ready for review July 13, 2020 18:28
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

@Razican
Copy link
Member

Razican commented Jul 15, 2020

The error is giving is due to a missing cargo update.

@HalidOdat HalidOdat merged commit e49be36 into boa-dev:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants