744814 - re-evaluate cost of retaining Gay's dtoa code

Status: RESOLVED DUPLICATE of bug 1691998

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

bug 614188 removed js/src/v8-dtoa; the README in that directory read, in part:

----------
Two related functions in V8 are DoubleToExponentialCString() and
DoubleToPrecisionString(), which can be used to implement
Number.toExponential() and Number.toPrecision().  They have not been imported;
they both call dtoa() and so are unlikely to be noticeably faster than the
existing SpiderMonkey equivalents.

Another related function in V8 is DoubleToRadixCString(), which can be used to
implement Number.toString(base), where base != 10.  This has not been imported;
it may well be faster than SpiderMonkey's implementation, but V8 generates its
own definition of the modulo() function on Win64 and importing this would
require also importing large chunks of the assembler, which is not worthwhile.

Yet another related function in V8 is DoubleToFixedCString(), which can be used
to implement Number.toFixed().  This has not been imported as it was measured
as slower than SpiderMonkey's version.
----------

We keep Gay's dtoa code around for the first two cases.  We now have double-conversion, which can be used to implement the former, and it's not clear if non-radix 10 conversions are important enough to keep dtoa around for the latter.

We should re-evaluate keeping Gay's dtoa code, which has a codesize and maintenance cost, possibly a performance one as well.

Comment 1

[Luke Wagner [:luke]]

Bug 614188 mentioned that Google also opted to remove Gay's dtoa code.

Comment 2

[Luke Wagner [:luke]]

Nathan: I'm not quite familiar with where everything plugs in together, but if you could provide a starting patch that removes dtoa.c, I'll measure and review it.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Luke Wagner [:luke] from comment #2)
> Nathan: I'm not quite familiar with where everything plugs in together, but
> if you could provide a starting patch that removes dtoa.c, I'll measure and
> review it.

Cool, thanks.  I'll poke at that early in the next release cycle.

Comment 4

[Dan Gohman [:sunfish]]

Bug 1058069 has patches which update dtoa.c to the latest upstream version, which might be interesting if someone wanted to do some benchmarking.