Last Comment Bug 614188 - Import Google's double-conversion into mozilla/mfbt
: Import Google's double-conversion into mozilla/mfbt
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla14
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
: 736548 (view as bug list)
Depends on: 743988 745762
Blocks: 608915 750620
  Show dependency treegraph
 
Reported: 2010-11-23 00:49 PST by Jan de Mooij [:jandem] (PTO until July 31)
Modified: 2012-05-19 09:16 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add double-conversion to MFBT (238.58 KB, patch)
2012-03-08 13:38 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
use double-conversion in the JS engine (223.51 KB, patch)
2012-03-08 13:41 PST, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
part 1 - setup mftb/double-conversion (7.40 KB, patch)
2012-03-19 11:41 PDT, Nathan Froyd [:froydnj]
jwalden+bmo: review+
Details | Diff | Splinter Review
part 2 - import double-conversion sources (239.72 KB, patch)
2012-03-19 11:43 PDT, Nathan Froyd [:froydnj]
jwalden+bmo: review+
Details | Diff | Splinter Review
part 3 - build double-conversion as a part of MFBT (675 bytes, patch)
2012-03-19 11:44 PDT, Nathan Froyd [:froydnj]
jwalden+bmo: review+
khuey: review-
Details | Diff | Splinter Review
part 1 - setup mftb/double-conversion (7.40 KB, patch)
2012-04-06 11:55 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 2 - import double-conversion sources (239.72 KB, patch)
2012-04-06 11:56 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
part 3 - build double-conversion as a part of MFBT (2.49 KB, patch)
2012-04-06 11:58 PDT, Nathan Froyd [:froydnj]
khuey: review+
Details | Diff | Splinter Review
part 3b - make the JS engine use double_conversion (3.09 KB, patch)
2012-04-06 11:59 PDT, Nathan Froyd [:froydnj]
jwalden+bmo: review+
Details | Diff | Splinter Review
part 3c - delete v8-dtoa code (220.71 KB, patch)
2012-04-06 12:02 PDT, Nathan Froyd [:froydnj]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] (PTO until July 31) 2010-11-23 00:49:38 PST
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 [:njn] 2010-11-23 00:55:54 PST
(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] (PTO until July 31) 2010-11-23 01:09:44 PST
(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 [:bz] 2010-11-23 05:20:34 PST
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] (PTO until July 31) 2010-11-23 06:11:24 PST
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] (PTO until July 31) 2011-04-16 11:31:45 PDT
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 [:bz] 2011-04-21 09:08:41 PDT
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 2011-04-23 04:39:00 PDT
(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 [:bz] 2011-04-27 20:37:26 PDT
Note to self: it's mozilla/mfbt.

Taking this, I guess.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 21:20:42 PDT
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] 2012-03-08 13:38:59 PST
Created attachment 604182 [details] [diff] [review]
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] 2012-03-08 13:41:29 PST
Created attachment 604183 [details] [diff] [review]
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 [:bz] 2012-03-09 14:14:29 PST
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] (remove +bmo to email) 2012-03-15 16:51:24 PDT
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] (remove +bmo to email) 2012-03-15 16:51:58 PDT
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 [:bz] 2012-03-16 11:47:59 PDT
Nathan, since you're working on this, want to just take the bug?
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-03-16 14:35:47 PDT
*** Bug 736548 has been marked as a duplicate of this bug. ***
Comment 17 Nathan Froyd [:froydnj] 2012-03-19 06:34:52 PDT
(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] 2012-03-19 06:36:15 PDT
OK, some of those changes were clearly bogus...
Comment 19 Boris Zbarsky [:bz] 2012-03-19 06:49:52 PDT
I pretty much never mind someone stealing my bugs.  ;)
Comment 20 Nathan Froyd [:froydnj] 2012-03-19 11:41:52 PDT
Created attachment 607232 [details] [diff] [review]
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] 2012-03-19 11:43:06 PDT
Created attachment 607233 [details] [diff] [review]
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] 2012-03-19 11:44:24 PDT
Created attachment 607235 [details] [diff] [review]
part 3 - build double-conversion as a part of MFBT

...and this patch adds the necessary build bits to MFBT.
Comment 23 Nicholas Nethercote [:njn] 2012-03-19 12:51:00 PDT
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] (remove +bmo to email) 2012-03-29 17:50:07 PDT
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] (remove +bmo to email) 2012-03-29 17:54:19 PDT
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 2012-03-30 01:50:55 PDT
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] 2012-03-30 07:26:28 PDT
(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 :Ehsan Akhgari 2012-03-30 09:17:53 PDT
(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 [:khuey] (khuey@mozilla.com) 2012-04-03 08:30:43 PDT
This almost certainly does not work for standalone shell builds.
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-03 08:35:32 PDT
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] 2012-04-04 12:00:52 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] (remove +bmo to email) 2012-04-04 12:06:56 PDT
(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] 2012-04-04 12:13:48 PDT
khuey voted for #1 on IRC, so I'm going to run with that.
Comment 34 Nathan Froyd [:froydnj] 2012-04-06 11:55:30 PDT
Created attachment 612960 [details] [diff] [review]
part 1 - setup mftb/double-conversion

Addressing Waldo's review comment.
Comment 35 Nathan Froyd [:froydnj] 2012-04-06 11:56:12 PDT
Created attachment 612961 [details] [diff] [review]
part 2 - import double-conversion sources

Need to import the sources with the new patch from part 1.
Comment 36 Nathan Froyd [:froydnj] 2012-04-06 11:58:07 PDT
Created attachment 612965 [details] [diff] [review]
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] 2012-04-06 11:59:47 PDT
Created attachment 612966 [details] [diff] [review]
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] 2012-04-06 12:02:12 PDT
Created attachment 612969 [details] [diff] [review]
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 [:khuey] (khuey@mozilla.com) 2012-04-06 13:21:15 PDT
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] 2012-04-09 11:29:39 PDT
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 42 Luke Wagner [:luke] 2012-04-12 09:28:19 PDT
(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] 2012-04-12 09:54:05 PDT
(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] 2012-04-12 09:57:44 PDT
Thanks!  Also, \o/
Comment 45 Boris Zbarsky [:bz] 2012-04-12 10:09:18 PDT
Nathan, thanks again for picking this up and running with it!

Note You need to log in before you can comment on or make changes to this bug.