Import Google's double-conversion into mozilla/mfbt

RESOLVED FIXED in mozilla14

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: jandem, Assigned: froydnj)

Tracking

({perf})

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

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?
(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?
(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?
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.  ;)
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 ;)
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
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...
Blocks: 608915
(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.
Note to self: it's mozilla/mfbt.

Taking this, I guess.
Assignee: general → bzbarsky
Priority: -- → P1
Summary: Consider updating fast-dtoa code and removing Gay's dtoa → Import Google's double-conversion into mozilla/mfbt
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 .
Component: JavaScript Engine → MFBT
QA Contact: general → mfbt
Posted patch add double-conversion to MFBT (obsolete) — Splinter Review
Here's a patch that imports double-conversion, or at least the important source files themselves.
Attachment #604182 - Flags: feedback?(jwalden+bmo)
...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?).
Attachment #604183 - Flags: feedback?(jwalden+bmo)
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 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".
Attachment #604182 - Flags: feedback?(jwalden+bmo)
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.
Attachment #604183 - Flags: feedback?(jwalden+bmo)
Nathan, since you're working on this, want to just take the bug?
Duplicate of this bug: 736548
(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... ;)
Assignee: bzbarsky → nfroyd
Component: MFBT → Localization
QA Contact: mfbt → localization
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
OK, some of those changes were clearly bogus...
Assignee: nfroyd → nobody
Component: Localization → MFBT
QA Contact: localization → mfbt
Target Milestone: mozilla13 → ---
Assignee: nobody → nfroyd
I pretty much never mind someone stealing my bugs.  ;)
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.
Attachment #604182 - Attachment is obsolete: true
Attachment #607232 - Flags: review?(jwalden+bmo)
This patch is the result of running our shiny new update.sh script against double-conversion v1.1.  Nothing much to see here.
Attachment #607233 - Flags: review?(jwalden+bmo)
...and this patch adds the necessary build bits to MFBT.
Attachment #607235 - Flags: review?(jwalden+bmo)
As the original (hacky) importer of the v8-dtoa code, I'm very happy to see this bug moving forward.  Thanks, Nathan!
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&)
Attachment #607232 - Flags: review?(jwalden+bmo) → review+
Attachment #607233 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #607235 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #607235 - Flags: review?(khuey)
(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?
(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!
This almost certainly does not work for standalone shell builds.
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.
Attachment #607235 - Flags: review?(khuey) → review-
(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.
(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.
khuey voted for #1 on IRC, so I'm going to run with that.
Addressing Waldo's review comment.
Attachment #607232 - Attachment is obsolete: true
Attachment #612960 - Flags: review+
Need to import the sources with the new patch from part 1.
Attachment #607233 - Attachment is obsolete: true
Attachment #612961 - Flags: review+
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.
Attachment #607235 - Attachment is obsolete: true
Attachment #612965 - Flags: review?(khuey)
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.
Attachment #604183 - Attachment is obsolete: true
Attachment #612966 - Flags: review?(jwalden+bmo)
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.
Attachment #612969 - Flags: review?(jwalden+bmo)
Attachment #612966 - Attachment is patch: true
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.
Attachment #612965 - Flags: review?(khuey) → review+
Attachment #612966 - Flags: review?(jwalden+bmo) → review+
Attachment #612969 - Flags: review?(jwalden+bmo) → review+
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?
Depends on: 743988
(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.
(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.
Thanks!  Also, \o/
Nathan, thanks again for picking this up and running with it!
Depends on: 745762
Blocks: 750620
No longer blocks: 750620
Blocks: 750620
You need to log in before you can comment on or make changes to this bug.