Import Google's double-conversion into mozilla/mfbt

RESOLVED FIXED in mozilla14

Status

()

Core
MFBT
P1
normal
RESOLVED FIXED
7 years ago
5 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)

(Reporter)

Description

7 years ago
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?
(Reporter)

Comment 2

7 years ago
(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.  ;)
(Reporter)

Comment 4

7 years ago
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 ;)
(Reporter)

Comment 5

6 years ago
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
Keywords: perf
(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
(Assignee)

Comment 10

5 years ago
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.
Attachment #604182 - Flags: feedback?(jwalden+bmo)
(Assignee)

Comment 11

5 years ago
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?).
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
(Assignee)

Comment 17

5 years ago
(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
(Assignee)

Comment 18

5 years ago
OK, some of those changes were clearly bogus...
Assignee: nfroyd → nobody
Component: Localization → MFBT
QA Contact: localization → mfbt
Target Milestone: mozilla13 → ---
(Assignee)

Updated

5 years ago
Assignee: nobody → nfroyd
I pretty much never mind someone stealing my bugs.  ;)
(Assignee)

Comment 20

5 years ago
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.
Attachment #604182 - Attachment is obsolete: true
Attachment #607232 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 21

5 years ago
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.
Attachment #607233 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 22

5 years ago
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.
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)
(Assignee)

Comment 27

5 years ago
(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-
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 33

5 years ago
khuey voted for #1 on IRC, so I'm going to run with that.
(Assignee)

Comment 34

5 years ago
Created attachment 612960 [details] [diff] [review]
part 1 - setup mftb/double-conversion

Addressing Waldo's review comment.
Attachment #607232 - Attachment is obsolete: true
Attachment #612960 - Flags: review+
(Assignee)

Comment 35

5 years ago
Created attachment 612961 [details] [diff] [review]
part 2 - import double-conversion sources

Need to import the sources with the new patch from part 1.
Attachment #607233 - Attachment is obsolete: true
Attachment #612961 - Flags: review+
(Assignee)

Comment 36

5 years ago
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.
Attachment #607235 - Attachment is obsolete: true
Attachment #612965 - Flags: review?(khuey)
(Assignee)

Comment 37

5 years ago
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.
Attachment #604183 - Attachment is obsolete: true
Attachment #612966 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 38

5 years ago
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.
Attachment #612969 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 40

5 years ago
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?
(Assignee)

Updated

5 years ago
Depends on: 743988
(Assignee)

Comment 41

5 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla14

Comment 42

5 years ago
(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.
(Assignee)

Comment 43

5 years ago
(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

5 years ago
Thanks!  Also, \o/
Nathan, thanks again for picking this up and running with it!
https://hg.mozilla.org/mozilla-central/rev/463626bffba7
https://hg.mozilla.org/mozilla-central/rev/1603db611224
https://hg.mozilla.org/mozilla-central/rev/e17e02a3d5d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
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.