854531 - Import WebCore's Decimal class

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jonathan Watt [:jwatt]]

To fix bug 853525, I need a BCD type. I've looked into various BCD libs that we can import, and by far the simplest solution seems to be to import WebCore's Decimal class. I've managed to get its source and header file building in the Mozilla tree without any other webkit resources.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Initial import

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Remove webkit dependencies and add to Mozilla build system

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Created attachment 729128 [details] [diff] [review]
> Remove webkit dependencies and add to Mozilla build system

Can you please structure this more along the lines of mfbt/double-conversion/?  See, e.g. update.sh in that directory for pulling in a directory from WebKit and applying some suite of patches.  Bonus points if your patches use the double-conversion bits to implement conversions rather than std:: bits. ;)

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm going to leave these reviews to Waldo.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 1 - import r146754 of WebCore's Decimal class

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 2 - add an update.sh script for updating the mftt/decimal directory

Comment 7

[Jonathan Watt [:jwatt]]

Updated the "This was last updated with..." line locally.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 729685 [details] [diff] [review]
part 1 - import r146754 of WebCore's Decimal class

Review of attachment 729685 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.  But can we just import this stuff?  Are the licenses compatible enough to do so?  Paging Gerv to figure that out, I guess...

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 729690 [details] [diff] [review]
part 2 - add an update.sh script for updating the mftt/decimal directory

Review of attachment 729690 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine enough if the license questions can be answered positively.

Comment 10

[Jeff Walden [:Waldo]]

Oh, what's the plan for undoing the WTF and <stdint.h> dependencies?  Extra patches atop, probably?

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Oh, what's the plan for undoing the WTF and <stdint.h> dependencies?  Extra
> patches atop, probably?

Yes. Coming up once I've got the nsHTMLInputElement changes working and know that I'm not going to change those patches any more.

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 729685 [details] [diff] [review]
part 1 - import r146754 of WebCore's Decimal class

r=gerv. This licence is already in about:license.

Gerv

Comment 13

[Jonathan Watt [:jwatt]]

Upstream bug for bugs discovered in Decimal::floor and Decimal::ceil:

http://code.google.com/p/chromium/issues/detail?id=234323

Comment 14

[Jonathan Watt [:jwatt]]

Upstream bug for bug discovered with Decimal::toString:

http://code.google.com/p/chromium/issues/detail?id=234349

Comment 15

[Jonathan Watt [:jwatt]]

I'd been hoping to get my bug fixes for the Decimal code reviewed upstream so that I could just pull them down and save any Mozilla reviewers from having to spend time looking at them. That's not happening fast though, and I think we're out of time for that process since we need this for v23. So unfortunately I'm just going to have to go ahead and add them as a series of update.sh patches to be reviewed here. :/

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: part 3 - fix bug in Decimal::floor/ceiling

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: part 4 - fix bug in Decimal serialization

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: part 5 - fix the handling of NaN in Decimal::operator== and Decimal::operator!=

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: part 6 - add MFBT_API markers to Decimal methods

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: part 7 - use Mozilla/std dependencies instead of Blink core dependencies

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: part 8 - enable building of mfbt/decimal

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: part 9 - changes to Decimal code as a series of patches for update.sh

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 743686 [details] [diff] [review]
part 3 - fix bug in Decimal::floor/ceiling

Review of attachment 743686 [details] [diff] [review]:
-----------------------------------------------------------------

I'll feel better when upstream reviews this, but I think this all tracks logically.

::: mfbt/decimal/Decimal.cpp
@@ +630,5 @@
>          return isPositive() ? Decimal(1) : zero(Positive);
>  
> +    uint64_t result = scaleDown(coefficient, numberOfDropDigits);
> +    uint64_t droppedDigets = coefficient - scaleUp(result, numberOfDropDigits);
> +    if (droppedDigets && isPositive())

Typo: droppedDigits.

@@ +672,5 @@
>          return isPositive() ? zero(Positive) : Decimal(-1);
>  
> +    uint64_t result = scaleDown(coefficient, numberOfDropDigits);
> +    uint64_t droppedDigets = coefficient - scaleUp(result, numberOfDropDigits);
> +    if (droppedDigets && isNegative()) {

Typo again: droppedDigits.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 743687 [details] [diff] [review]
part 4 - fix bug in Decimal serialization

Review of attachment 743687 [details] [diff] [review]:
-----------------------------------------------------------------

I could imagine the API being designed the way it currently is, but probably not.  It's too bad there's no documentation to clarify expectations.  :-\

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 743690 [details] [diff] [review]
part 5 - fix the handling of NaN in Decimal::operator== and Decimal::operator!=

Review of attachment 743690 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like Decimal::operator<= and Decimal::operator>= have the same issue.  The fix here (and the maybe-nit) should apply to both those just as equally.  (r=me on changing those too, if it wasn't clear.)

::: mfbt/decimal/Decimal.cpp
@@ +515,1 @@
>      return m_data == rhs.m_data || compareTo(rhs).isZero();

I might also consider just removing the == check on the m_data (both places), but I guess upstream can sort out what it wants, and what the perf implications are, if any.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 743692 [details] [diff] [review]
part 6 - add MFBT_API markers to Decimal methods

Review of attachment 743692 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to assume this adds to enough/all the right places, and not check whether they were all hit or not.  :-)  I imagine missing a place would be discovered right quick when building, so whatever.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 743693 [details] [diff] [review]
part 7 - use Mozilla/std dependencies instead of Blink core dependencies

Review of attachment 743693 [details] [diff] [review]:
-----------------------------------------------------------------

Minor nitpicks, nothing serious enough that I want a second look.

::: mfbt/decimal/Decimal.h
@@ +41,3 @@
>  #include "mozilla/Types.h"
>  
>  #include <stdint.h>

Until bug 866425 is fixed, this needs to be "mozilla/StandardInteger.h".

@@ +45,5 @@
> +
> +#ifndef ASSERT
> +#define DEFINED_ASSERT_FOR_DECIMAL_H 1
> +#define ASSERT MOZ_ASSERT
> +#endif

ASSERT is also defined by <windows.h>, so letting this header's assertions (I see Decimal.cpp is fixed in this regard) use ASSERT (if a Windows header was already included) makes me a little leery, as far as the behavior that will occur when asserting.  I don't know that our tinderbox setups and all can handle the message format/behavior of <windows.h> ASSERT, although I imagine the builds abort/crash in some vaguely sensible way with it.

For that reason I'd slightly prefer s/ASSERT/MOZ_ASSERT/, but that's mostly inchoate fears, so I can live with this til an issue actually manifests.

::: mfbt/decimal/moz-decimal-utils.h
@@ +17,5 @@
> +
> +#ifndef UINT64_C
> +// For Android toolchain
> +#define UINT64_C(c) (c ## ULL)
> +#endif

Hmm.  I guess I'm going to have to update my patches for bug 730805 for this, assuming I ever figure out the Windows PGO story for its bug 812218 prerequisite.  Oh well, fair enough in the meantime.

@@ +29,5 @@
> +
> +#define WTF_MAKE_NONCOPYABLE(ClassName) \
> +  private: \
> +    ClassName(const ClassName&) MOZ_DELETE; \
> +    ClassName& operator=(const ClassName&) MOZ_DELETE;

Making the latter return |void| is perhaps slightly nicer, because then non-=delete compilers will get a compile error accidentally invoking operator=, not just a link error.

@@ +36,5 @@
> +namespace std {
> +  inline bool isinf(double num) { return !_finite(num) && !_isnan(num); }
> +  inline bool isnan(double num) { return !!_isnan(num); }
> +  inline bool isfinite(double x) { return _finite(x); }
> +  inline bool signbit(double num) { return _copysign(1.0, num) < 0; }

Hmm.  Why not use the FloatingPoint.h methods for these cases?  We know those are PGO-safe, but there have been issues in the past with the MSVC methods in Windows PGO builds, so FloatingPoint.h is preferable.

@@ +69,5 @@
> +  double_conversion::StringToDoubleConverter converter(
> +    double_conversion::StringToDoubleConverter::NO_FLAGS,
> +    MOZ_DOUBLE_NaN(), MOZ_DOUBLE_NaN(), nullptr, nullptr);
> +  const char* str = aStr.c_str();
> +  int length = strlen(str);

I haven't blogged about the new header/method yet, but this could use mozilla/Casting.h and |SafeCast<int>(strlen(str))| to verify that the size_t here will actually fit in an int.

@@ +78,5 @@
> +}
> +
> +String mozToString(double aNum) {
> +  char buffer[64];
> +  int buffer_length = sizeof(buffer) / sizeof(char);

mozilla::ArrayLength

@@ +87,5 @@
> +  return String(builder.Finalize());
> +}
> +
> +String mozToString(uint64_t aNum) {
> +  return mozToString(double(aNum));

There's no requirement that |aNum > 2**52ish| have precise stringification, correct?

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 743696 [details] [diff] [review]
part 9 - changes to Decimal code as a series of patches for update.sh

Review of attachment 743696 [details] [diff] [review]:
-----------------------------------------------------------------

This'll need updating for whatever previous patch changes I noted, that you make, but whatever, looks good.  :-)

Comment 29

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf94844ab83f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc31f6926e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbed5d03005
https://hg.mozilla.org/integration/mozilla-inbound/rev/574bda36a38c
https://hg.mozilla.org/integration/mozilla-inbound/rev/101889486438
https://hg.mozilla.org/integration/mozilla-inbound/rev/84eb9e72b338
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca314d5783b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2a11eb7db0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40200a44162

Comment 30

[Jonathan Watt [:jwatt]]

Attachment: tweak to part 7

To work around bug 868814 (Windows opt crash) I made this additional change to part 7 to add a Decimal::toString method with a char* outparam.

Comment 31

[Jonathan Watt [:jwatt]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> I could imagine the API being designed the way it currently is, but probably
> not.  It's too bad there's no documentation to clarify expectations.  :-\

I hope it wasn't designed to make:

Decimal a, b, c;
...
a = b - c;
a.toString() == "0.00"; // if it happens a == b, and the exponent of both is -2

That would be sucky design. ;)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27)
> > +String mozToString(uint64_t aNum) {
> > +  return mozToString(double(aNum));
> 
> There's no requirement that |aNum > 2**52ish| have precise stringification,
> correct?

Actually there might be, so I added two extra mozToString functions with uint64_t and int64_t args, implemented using std::ostringstream, as we discussed on IRC. I doubt using ostringstream is significant on perf, but if it shows up we can figure something out.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf94844ab83f
https://hg.mozilla.org/mozilla-central/rev/cc31f6926e56
https://hg.mozilla.org/mozilla-central/rev/4bbed5d03005
https://hg.mozilla.org/mozilla-central/rev/574bda36a38c
https://hg.mozilla.org/mozilla-central/rev/101889486438
https://hg.mozilla.org/mozilla-central/rev/84eb9e72b338
https://hg.mozilla.org/mozilla-central/rev/1ca314d5783b
https://hg.mozilla.org/mozilla-central/rev/1c2a11eb7db0
https://hg.mozilla.org/mozilla-central/rev/f40200a44162

Comment 33

[Jonathan Watt [:jwatt]]

FWIW Dietmar Kühl gives an update on his work to get native decimal added to C++17 here:

http://stackoverflow.com/questions/14096026/c-decimal-data-types

The actual text of the latest proposal is here:

http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2012/n3407.html