576934 - parseInt(double) is slow when not tracing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alan Pierce]

From bug 454037, parseInt(double) is needlessly slow because it converts to a string and back. The patch for that bug only fixed the case when we're on trace, but we should change num_parseInt to behave that way, too.

This helps in SunSpider's string-unpack-code, which does parseInt(c / a) about 10,000 times as a way of doing integer division. On moo, this change saves us about 10% to 20% of string-unpack-code, which is 1% to 2% of sunspider as a whole.

Comment 1

[Alan Pierce]

Attachment: SunSpider on moo before and after

Everything except for unpack-code can probably be ignored.

Comment 2

[Alan Pierce]

Attachment: Special-case parseInt(double) in num_parseInt

This is for the moo branch.

Comment 3

[Brendan Eich [:brendan]]

Very minor (and pre-existing) nit:

+static inline jsdouble
+DoubleToInt(jsdouble d)

returns an integral double, so DoubleToInteger would be better. We (and IIRC the ECMA-262 spec) spell out "Integer" to mean integral double, while "Int" usually means jsint/int32.

/be

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 456024 [details] [diff] [review]
Special-case parseInt(double) in num_parseInt

Nice find. r=me (w/ brendan's nits :)

Comment 5

[Alan Pierce]

Pushed: http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/9b05f8339fb1

Comment 6

[Alan Pierce]

Attachment: Patch

Looks like this change got accidentally removed when the fix for bug 577536 was merged from tracemonkey to moo. Here's a redo of the patch, updated for the surrounding code.

This is for tracemonkey, since it looks like otherwise it would need another manual merge at the next merge from TM to moo, since the code has been updated again since the last merge.

Comment 7

[David Mandelin [:dmandelin]]

Thanks for catching this. Relanded to JM:

http://hg.mozilla.org/projects/jaegermonkey/rev/842dce90c28e

Getting it landed on TM is still a good idea.

Comment 8

[Paul Wright]

Based on the SS #s in this bug, it appears to be a clear win.  If you look at AWFY, rev 89964b93d40d (before) and rev a2f760bfd2bd (after) are virtually identical in perf.  Any thoughts as to why?

Comment 9

[Alan Pierce]

(In reply to comment #8)
> Based on the SS #s in this bug, it appears to be a clear win.  If you look at
> AWFY, rev 89964b93d40d (before) and rev a2f760bfd2bd (after) are virtually
> identical in perf.  Any thoughts as to why?

This was originally landed on July 6, which is off the graph on AWFY. It was a clear perf win on the graph at the time. The change was accidentally clobbered after 89964b93d40d, and then relanded before a2f760bfd2bd, so the big perf regression from the clobber doesn't show up on AWFY.

Comment 10

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/mozilla-central/rev/842dce90c28e