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

float to string conversions have rounding errors due to trying to produce too many digits #17205

Closed
bzbarsky opened this issue Jun 7, 2017 · 9 comments

Comments

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Jun 7, 2017

32-bit float can realistically give you 6 decimal digits (total, not "after the decimal point") of precision (in the sense that 6-digit strings round-trip when going to float and back). But the normal std::fmt conversion will try to give you 9-digit output (because 9-digit strings can represent floats with perfect fidelity).

This comes up in the following testcase:

<pre><script>
   var d = document.createElement("div");
   for (var i = 0; i < 1000; ++i) {
     d.style.width = (i/10) + "%";
     document.writeln((i/10), "% ", d.getAttribute("style").slice(7));
   }
</script>

In Gecko, Blink, WebKit, all the lines have two identical numbers on them. With stylo and servo, some of the lines do not:

15% 15.000001%;
27% 27.000002%;
30% 30.000002%;
53% 52.999996%;
54% 54.000004%;
59% 58.999996%;
60% 60.000004%;

What Gecko does here, for what it's worth, is format to 6 digits total precision, then trim off trailing '0' and '.' as needed. This is the normal "32 bit float to string" conversion in Gecko.

This is causing mochitest failures for stylo; see https://bugzilla.mozilla.org/show_bug.cgi?id=1370779

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Jun 7, 2017

@nox, @Manishearth thought you might be interested in this one.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jun 7, 2017

Note, the 6-digit requirement is only for truncating the decimal, I suppose 600000001.0 will serialize with the 1.

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Jun 7, 2017

In Gecko and Chrome (which use the same double formatting library) it serializes as "6e+08", which is of course not so helpful. Safari does "600000001", though.

@jdm jdm added the A-stylo label Jun 7, 2017
@highfive
Copy link

@highfive highfive commented Jun 7, 2017

cc @emilio

@jdm jdm added the A-content/css label Jun 7, 2017
@nox
Copy link
Member

@nox nox commented Jun 7, 2017

Seems like dtoa is related to the same algorithm that Gecko uses.

@upsuper
Copy link
Member

@upsuper upsuper commented Jul 24, 2017

OK, I have a working patch to fix this... It may need some more test coverage since it's nontrivial and I'm not confident enough on myself for this kind of change...

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jul 28, 2017

This is specific to percentages. We divide the value of percentage tokens by 100.0 in the parser, and multiply it by 100.0 in the serializer. (15_f32 / 100.) * 100. does not equal 15_f32. Maybe we could move this division later in the pipeline?

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Jul 28, 2017

The basic question is what the thing is that we want to preserve.

The current behavior assumes that the floating point value is the thing we care about and that serializing should preserve it with as much fidelity as possible. But from the point of view of a web author the internal floating-point representation is not something they care about. Furthermore, it's commonly lossy from the web developer's point of view in all sorts of cases. That includes not just percentages, but various calc() cases, computed style cases, etc.

The other option, and what I think we should do, is to aim at preserving "the original string representation" when serializing. This is impossible in general, of course, because lots of strings map to the same float. But all 6-digit decimal strings can be encoded in distinct floats. So if we always serialize to a 6-digit string, we may produce something that parses to a different float value (or may not, depending on what processing happens in addition to the parsing), but we are more likely to produce something that round-trips the actual original input.

bors-servo added a commit to servo/rust-cssparser that referenced this issue Aug 18, 2017
Serialize number to no more than necessary precision

I'm not sure whether it can be simplified... and I probably need some more idea for how should I test it... But things look fine so far.

This should probably goes to an independent crate at some point.

It should fix servo/servo#17205.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/173)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 18, 2017

Reopening since this has yet to land in Servo.

@SimonSapin SimonSapin reopened this Aug 18, 2017
SimonSapin added a commit that referenced this issue Aug 23, 2017
Pull in servo/rust-cssparser#173,
which together with #18159 fixes #17205
bors-servo added a commit that referenced this issue Aug 23, 2017
Update cssparser to 0.19.2

Pull in servo/rust-cssparser#173, which together with #18159 fixes #17205.
bors-servo added a commit that referenced this issue Aug 23, 2017
Update cssparser to 0.19.2

Pull in servo/rust-cssparser#173, which together with #18159 fixes #17205.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18201)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 23, 2017
Update cssparser to 0.19.2

Pull in servo/rust-cssparser#173, which together with #18159 fixes #17205.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18201)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.