Closed Bug 576934 Opened 11 years ago Closed 11 years ago

parseInt(double) is slow when not tracing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: alangpierce, Assigned: alangpierce)

Details

Attachments

(2 files, 1 obsolete file)

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.
Everything except for unpack-code can probably be ignored.
Assignee: general → apierce
Status: NEW → ASSIGNED
This is for the moo branch.
Attachment #456024 - Flags: review?(dvander)
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 on attachment 456024 [details] [diff] [review]
Special-case parseInt(double) in num_parseInt

Nice find. r=me (w/ brendan's nits :)
Attachment #456024 - Flags: review?(dvander) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attached patch PatchSplinter Review
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.
Attachment #456024 - Attachment is obsolete: true
Attachment #462343 - Flags: review?(jwalden+bmo)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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?
Attachment #462343 - Flags: review?(jwalden+bmo)
(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.
http://hg.mozilla.org/mozilla-central/rev/842dce90c28e
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.