614188 - Import Google's double-conversion into mozilla/mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect, P1)

Description

[Jan de Mooij [:jandem]]

V8 recently added a bignum fallback, which enabled them to remove Gay's dtoa entirely.

http://code.google.com/p/v8/source/detail?r=5840
http://code.google.com/p/v8/source/detail?r=5868

Is it possible for us to do the same by updating our copy of the string <-> double conversion code?

Comment 1

[Nicholas Nethercote [inactive]]

(In reply to comment #0)
> 
> Is it possible for us to do the same by updating our copy of the string <->
> double conversion code?

Probably.  Are there clear benefits to doing this?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to comment #1)
> Probably.  Are there clear benefits to doing this?

Benefits are less obscure code and I assume better performance, but I have not tested or verified that. We could compare V8 revisions before and after the bignum fallback. Are there many cases the fast-dtoa code does not handle?

Comment 3

[Boris Zbarsky [:bzbarsky]]

In an ideal world we'd have one fast dtoa that we can use from both jseng and core Gecko.  If dropping dtoa.c will help us get there, I'm all for it.  ;)

Comment 4

[Jan de Mooij [:jandem]]

I had an e-mail conversation with the author, part of his reply (posting here with permission):

"The remaining work, and in particular the bignum implementation, has been developed for code maintenance. I haven't measured any noticeable speed differences between Gay's dtoa and the new implementation.
Note, that the bignum implementation itself is very basic and not optimized. Replacing it with gmp would clearly make the whole operation much faster. But since the bignum case is the slow case which is only executed in 0.5% of the time we didn't feel the need to optimize it anymore. And we didn't want to depend on another library.

Also, I've started to extract the dtoa/strtod parts of V8 so they can be easily used in other projects. I don't know yet when I'm going to finish the library, but feel free to ping me once a month so I don't forget."

I guess we should just wait for that awesome dtoa-library then ;)

Comment 5

[Jan de Mooij [:jandem]]

The V8 people have extracted the dtoa/strtod code from V8. It's now available as a separate project:

http://code.google.com/p/double-conversion

Comment 6

[Boris Zbarsky [:bzbarsky]]

Chris, do you think we can toss this V8 code into our new mozilla/tl thing?  I would _love_ to be able to use this in things like nsString::AppendFloat and the like...

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #6)
> Chris, do you think we can toss this V8 code into our new mozilla/tl thing?

Yep, that's what it's there for.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note to self: it's mozilla/mfbt.

Taking this, I guess.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The only snag here is that mfbt/ code is built by js/src/Makefile.in, so you need to add the new stuff here http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#270 .

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: add double-conversion to MFBT

Here's a patch that imports double-conversion, or at least the important source files themselves.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: use double-conversion in the JS engine

...and here's the patch that makes the JS engine use it.  It's huge because it also removes v8-dtoa at the same time.

make check in js/src passes, but I am skeptical I got so big a change right the first time. :)

Note that we can't get rid of the dtoa.c code entirely since double-conversion doesn't support anything other than base 10.  But at least we have shared code now, and hopefully the new version is faster (though I have not benchmarked anything yet...suggestions?).

Comment 12

[Boris Zbarsky [:bzbarsky]]

It looks like you found the testcase in bug 608880 already.  If you're also changing number-to-string conversions, you could take a look at testcases that do things like:

  500.5 + "px"

or whatnot.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 604182 [details] [diff] [review]
add double-conversion to MFBT

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

::: mfbt/double-conversion/README
@@ +2,5 @@
> +
> +See http://code.google.com/p/double-conversion/ for more information.
> +
> +To import new versions, copy the source files and add MFBT_API
> +declarations as appropriate.

First, we should definitely be using the update.sh strategy lots of other in-tree imports use:

http://mxr.mozilla.org/mozilla-central/find?string=/update.sh

It's too easy for custom changes to become intermixed with imported code.  And I don't see how we're likely to gain from permanently forking this.

Second, the apparent number of changes to double-conversion.h is pretty big.  It seems preferable to me if we left that file untouched and just had a small wrapper class around it, both for API consistency and for update maintainability.

::: mfbt/double-conversion/utils.h
@@ +77,5 @@
> +typedef int int32_t;
> +typedef unsigned int uint32_t;
> +typedef __int64 int64_t;
> +typedef unsigned __int64 uint64_t;
> +// intptr_t and friends are defined in crtdefs.h through stdio.h.

These typedefs are a build conflict/error waiting to happen.  Use #include "mozilla/StandardInteger.h".

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 604183 [details] [diff] [review]
use double-conversion in the JS engine

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

This'll need updating for the changes I noted on the other patch, just a bit.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Nathan, since you're working on this, want to just take the bug?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Nathan, since you're working on this, want to just take the bug?

Well, as long as you don't mind me stealing bugs away from you... ;)

Comment 18

[Nathan Froyd [:froydnj]]

OK, some of those changes were clearly bogus...

Comment 19

[Boris Zbarsky [:bzbarsky]]

I pretty much never mind someone stealing my bugs.  ;)

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: part 1 - setup mftb/double-conversion

OK, new series of patches.  This first one simply initializes mfbt/double-conversion with an update.sh and the patches we will need to apply.  I don't think it's really worth writing wrapper classes if we're already going to be patching the sources, and the required changes for correct visibility are not onerous.  This patch is where all the issues should be; the other ones are trivial.

Comment 21

[Nathan Froyd [:froydnj]]

Attachment: part 2 - import double-conversion sources

This patch is the result of running our shiny new update.sh script against double-conversion v1.1.  Nothing much to see here.

Comment 22

[Nathan Froyd [:froydnj]]

Attachment: part 3 - build double-conversion as a part of MFBT

...and this patch adds the necessary build bits to MFBT.

Comment 23

[Nicholas Nethercote [inactive]]

As the original (hacky) importer of the v8-dtoa code, I'm very happy to see this bug moving forward.  Thanks, Nathan!

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 607232 [details] [diff] [review]
part 1 - setup mftb/double-conversion

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

::: mfbt/double-conversion/add-mfbt-api-markers.patch
@@ +14,5 @@
> +   }
> + 
> +   // Returns a converter following the EcmaScript specification.
> +-  static const DoubleToStringConverter& EcmaScriptConverter();
> ++  static const MFBT_API(DoubleToStringConverter&) EcmaScriptConverter();

This should be MFBT_API(const DoubleToStringConverter&)

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 607235 [details] [diff] [review]
part 3 - build double-conversion as a part of MFBT

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

::: mfbt/sources.mk
@@ +4,5 @@
> +  $(NULL)
> +
> +# Imported double-conversion sources.
> +VPATH += $(srcdir)/double-conversion \
> +  $(NULL)

Does this work correctly for browser builds and standalone JS engine builds both?  As a threshold matter, any mention of $(srcdir) has a funny smell to it.  If you've tested both, and both work, fine by me.

Comment 26

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

Comment on attachment 607235 [details] [diff] [review]
part 3 - build double-conversion as a part of MFBT

AIUI, using VPATH like this is frowned upon by build people.

Comment 27

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #25)
> Does this work correctly for browser builds and standalone JS engine builds
> both?  As a threshold matter, any mention of $(srcdir) has a funny smell to
> it.  If you've tested both, and both work, fine by me.

I have no idea how to do a standalone JS engine build.  Is it as simple as just ./configure'ing from the js/src directory in an m-c checkout?

Comment 28

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #27)
> (In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #25)
> > Does this work correctly for browser builds and standalone JS engine builds
> > both?  As a threshold matter, any mention of $(srcdir) has a funny smell to
> > it.  If you've tested both, and both work, fine by me.
> 
> I have no idea how to do a standalone JS engine build.  Is it as simple as
> just ./configure'ing from the js/src directory in an m-c checkout?

Yes!

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This almost certainly does not work for standalone shell builds.

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 607235 [details] [diff] [review]
part 3 - build double-conversion as a part of MFBT

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

r- since I'm pretty sure this doesn't work.  Rerequest review if it does.

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #30)
> r- since I'm pretty sure this doesn't work.  Rerequest review if it does.

Indeed.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #25)
> > +# Imported double-conversion sources.
> > +VPATH += $(srcdir)/double-conversion \
> > +  $(NULL)
> 
> Does this work correctly for browser builds and standalone JS engine builds
> both?  As a threshold matter, any mention of $(srcdir) has a funny smell to
> it.

Do you have ideas of how to properly handle this?  The two options that spring to mind are:

1) Require clients of sources.mk to set MFBT_ROOT or somesuch before including sources.mk.  This way we can get VPATH correct.  Mildly distasteful.
2) Instead of double-conversion/ (and other source directories for other (?) imported projects), just throw everything into mfbt/.  Punt, in effect.  Do not like.

Doing a standalone JS engine build also pointed out that some of the v8-dtoa files and the double-conversion files have the same names.  I suspect this will cause all sorts of interesting build issues.  I had hoped to avoid converting the JS engine to use double-conversion in this bug, but it looks as though it will have to happen.

Comment 32

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd (:froydnj) from comment #31)
> > Does this work correctly for browser builds and standalone JS engine builds
> > both?  As a threshold matter, any mention of $(srcdir) has a funny smell to
> > it.
> 
> Do you have ideas of how to properly handle this?  The two options that
> spring to mind are:
> 
> 1) Require clients of sources.mk to set MFBT_ROOT or somesuch before
> including sources.mk.  This way we can get VPATH correct.  Mildly
> distasteful.

I dunno if this is a good idea or a bad idea, because I don't know much about our build system, about build systems in general, and about what's better and worse in them.  Find someone else who knows this stuff.  :-)  glandium is probably a good bet.

> 2) Instead of double-conversion/ (and other source directories for other (?)
> imported projects), just throw everything into mfbt/.  Punt, in effect.  Do
> not like.

Don't do this.  It'd end up causing problems later, I think we're both pretty sure of that.

> Doing a standalone JS engine build also pointed out that some of the v8-dtoa
> files and the double-conversion files have the same names.  I suspect this
> will cause all sorts of interesting build issues.  I had hoped to avoid
> converting the JS engine to use double-conversion in this bug, but it looks
> as though it will have to happen.

Quite possible.  Given the small extent of JS code at issue here, it's probably for the best, actually.

Comment 33

[Nathan Froyd [:froydnj]]

khuey voted for #1 on IRC, so I'm going to run with that.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: part 1 - setup mftb/double-conversion

Addressing Waldo's review comment.

Comment 35

[Nathan Froyd [:froydnj]]

Attachment: part 2 - import double-conversion sources

Need to import the sources with the new patch from part 1.

Comment 36

[Nathan Froyd [:froydnj]]

Attachment: part 3 - build double-conversion as a part of MFBT

This implements the MFBT_ROOT idea so that standalone JS builds work.

The tree, however, *will not* build with this patch; the JS engine requires modifications.  For ease of reviewing, I've split those out into separate patches.  They will be rolled up with this one at commit time.

Comment 37

[Nathan Froyd [:froydnj]]

Attachment: part 3b - make the JS engine use double_conversion

This patch makes the appropriate modifications to the JS engine for jsnum.cpp to use double_conversion.  It should build, but it *will not* link due to name conflicts between v8-dtoa and double_conversion.  Separated out for ease of reviewing.

Comment 38

[Nathan Froyd [:froydnj]]

Attachment: part 3c - delete v8-dtoa code

Now that we're using double_conversion, we don't need the v8-dtoa code.

We can't nuke Gay's dtoa because we still use it for non-radix 10 conversions.  jsnum.cpp's comments suggest that v8's code (divide-and-append digits using fmod) is slower.  Perhaps we should open a separate bug to consider whether it's worth carrying around all that code for such cases.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 612965 [details] [diff] [review]
part 3 - build double-conversion as a part of MFBT

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

::: js/src/Makefile.in
@@ +450,5 @@
>  endif
>  
>  endif # JS_HAS_CTYPES
>  
> +MFBT_ROOT = $(srcdir)/../../mfbt

Can we move this into the standalone bit?

::: mfbt/sources.mk
@@ +1,2 @@
> +# Before including this file, you must define MFBT_ROOT in your Makefile to
> +# point to the MFBT source directory.

Please add an

ifndef MFBT_ROOT
$(error Before including this file ...)
endif

to the beginning of this file.

Comment 40

[Nathan Froyd [:froydnj]]

A try run has already started turning up interesting failures that I don't see locally:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10751048&tree=Try

libmozglue now includes symbols that can't be satisfied without linking with libm or libsupc++.  Linking stuff in tools/ with libsupc++ is dodgy, and let's not bother with libm unless we have to.

What's the right way out of this?

Comment 41

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/463626bffba7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1603db611224
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17e02a3d5d9

Looked good on try with bug 743988 patch: https://tbpl.mozilla.org/?tree=Try&rev=1d921e72d92a

Comment 42

[Luke Wagner [:luke]]

(In reply to Nathan Froyd (:froydnj) from comment #38)
> Perhaps we should open a separate bug to
> consider whether it's worth carrying around all that code for such cases.

Any bug filed?  It would be good to have a summary for why we still have dtoa.c and I suspect we may wish to re-measure and re-evaluate having both.

Comment 43

[Nathan Froyd [:froydnj]]

(In reply to Luke Wagner [:luke] from comment #42)
> (In reply to Nathan Froyd (:froydnj) from comment #38)
> > Perhaps we should open a separate bug to
> > consider whether it's worth carrying around all that code for such cases.
> 
> Any bug filed?  It would be good to have a summary for why we still have
> dtoa.c and I suspect we may wish to re-measure and re-evaluate having both.

This is now bug 744814.

Comment 44

[Luke Wagner [:luke]]

Thanks!  Also, \o/

Comment 45

[Boris Zbarsky [:bzbarsky]]

Nathan, thanks again for picking this up and running with it!

Comment 46

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/463626bffba7
https://hg.mozilla.org/mozilla-central/rev/1603db611224
https://hg.mozilla.org/mozilla-central/rev/e17e02a3d5d9