Optimize parseInt for integers in base-10

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: erik, Assigned: crowderbt)

Tracking

({perf, testcase})

Trunk
perf, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

13 years ago
I've seen parseInt being used quite a lot instead of Math.floor on Numbers. Since parseInt is more than twice as slow it might make sense to do a check to see if the type is Number and if so just use Math.floor instead.
(Reporter)

Comment 1

13 years ago
Created attachment 209121 [details]
Test case
(Reporter)

Comment 2

13 years ago
Maybe the same thing should be done for Rhino as well?
Keywords: testcase
(Reporter)

Comment 3

13 years ago
I realized that using (n | 0) is even faster. Why is that these 3 different ways to do the same thing have such a different performance?
(Reporter)

Comment 4

13 years ago
Created attachment 209129 [details]
Also tests bitwise or
Attachment #209121 - Attachment is obsolete: true
Math.floor and |0 don't do the same thing. Consider negative numbers.
Created attachment 209177 [details] [diff] [review]
possible fix

Should we burn some code space to make this case fast?  Is it really common?  How about JSVAL_IS_DOUBLE cases?

/be
Attachment #209177 - Flags: superreview?(shaver)
Attachment #209177 - Flags: review?(mrbkap)
Comment on attachment 209177 [details] [diff] [review]
possible fix

This looks good, though comment 0 seems to imply that the JSVAL_IS_DOUBLE case is something we should care about.
Attachment #209177 - Flags: review?(mrbkap) → review+
(Reporter)

Comment 8

13 years ago
I wasn't even aware that there was 2 different internal types for js numbers. I've seen parseInt being used for heavy math routines where Math.floor should be used. In these cases the numbers are usually doubles.
Keywords: perf

Comment 9

12 years ago
The patch doesn't check whether the radix is 10, for example the result of parseInt(11,3) should be 4, not 11.
Just MHO, but I don't think this is worth fixing. People doing 'heavy math routines' should know better than to abuse parseInt for rounding.
Very slow PC (~200MHz, 64RAM) results:
Testcase in attachments,

Opera:
parseInt: 19086
Math.floor: 9609
Bitwise or: 8078
Common page load speed = 48,194977

Mozilla:
parseInt: 119292
Math.floor: 108017
Bitwise or: 91086
Common page load speed = 401,432464

"opreport -l"  see in attachment
Created attachment 245662 [details]
gprofile  output for this testcase

gprof2html can convert this profile data to html view
Why not just attach the HTML?
(In reply to comment #12)
> Why not just attach the HTML?
> 

Because grpof2html will generate a lot of html's (it works so). 
I will attach gzipped folder with gprof2html result
Created attachment 245695 [details]
HTML output from grpof2html

Comment 15

12 years ago
Created attachment 245798 [details]
Various results win32

Various results for Erik's testcase on Win32 :

Windows 2003 SP1
P4 2.6 GHz w/ 512 MB

I will test on archlinux next week.

Updated

12 years ago
Attachment #245798 - Attachment description: Various result win32 → Various results win32

Comment 16

12 years ago
Comment on attachment 245798 [details]
Various results win32

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Yani's results With Erik's Tescase</title>
</head>
<body>
<ul>
<li>OS : Windows 2003 SP1</li>
<li>CPU : P4 2.6 GHz</li>
<li>MEM : 512 MB</li>
</ul>

<p>
<b>Firefox SSE2 11-14 :</b>
<br/>
parseInt: 2422
<br/>
Math.floor: 1515
<br/>
Bitwise or: 1297
</p>

<p>
<b>Firefox SSE2 11-24 :</b>
<br/>
parseInt: 2390
<br/>
Math.floor: 1610
<br/>
Bitwise or: 1375
</p>

<p>
<b>Internet Explorer 7.0.5730.11 (IE7 Final) :</b>
<br/>
parseInt: 1969
<br/>
Math.floor: 578
<br/>
Bitwise or: 391
</p>

<p>
<b>K-Meleon 1.02 (Gecko 1.8.0.7) :</b>
<br/>
parseInt: 3093
<br/>
Math.floor: 2282
<br/>
Bitwise or: 2015
</p>

<p>
<b>Netscape 8.1.2 (Gecko 1.7.5) : </b>
<br/>
parseInt: 1219
<br/>
Math.floor: 437
<br/>
Bitwise or: 438
</p>

<p>
<b>Opera 9.10.8649 :</b>
<br/>
parseInt: 1047
<br/>
Math.floor: 359
<br/>
Bitwise or: 266
</p>


<p>
<b>Opera 9.10.8660 :</b>
<br/>
parseInt: 1015
<br/>
Math.floor: 360
<br/>
Bitwise or: 265
</p>

</body>
</html>

Comment 17

12 years ago
Created attachment 246515 [details]
Various results win32

oops,sorry for bugspam.
Attachment #245798 - Attachment is obsolete: true

Updated

12 years ago
Blocks: 117611

Updated

11 years ago
Attachment #246515 - Attachment is obsolete: true

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Comment 18

11 years ago
Yes, this should block 1.9, I'll own it.  Looks like all we need is for "possible fix" to be updated to 1.9 -- would be nice to know before landing if it has any appreciable results in our benchmarks, though.  Sayrer?
Assignee: general → crowder
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 19

11 years ago
Created attachment 305126 [details] [diff] [review]
Update to current trunk

SunSpider results coming...
Attachment #209177 - Attachment is obsolete: true
(Assignee)

Comment 20

11 years ago
Not a huge difference in SunSpider results for any given test; nothing is dramatically worse, overall, a bit faster (20ms overall).  I'll let someone else do proper testing.
Flags: tracking1.9+ → blocking1.9+
Priority: -- → P2
(Assignee)

Comment 21

11 years ago
Comment on attachment 305126 [details] [diff] [review]
Update to current trunk

I think this patch only satisfies benchmarks and silly people, but....
Attachment #305126 - Flags: review?(mrbkap)
(Assignee)

Comment 22

11 years ago
And yes, I realize that comment 18 is me being a silly person.
Comment on attachment 305126 [details] [diff] [review]
Update to current trunk

>Index: jsnum.
>+    jsint value;

Make this a jsval.
Attachment #305126 - Flags: review?(mrbkap) → review+

Comment 24

11 years ago
(In reply to comment #21)
> (From update of attachment 305126 [details] [diff] [review])
> I think this patch only satisfies benchmarks and silly people, but....

But what? Just say it, it's pointless. And buggy, as I had already pointed out in comment 9.

Comment on attachment 305126 [details] [diff] [review]
Update to current trunk

Seno is right, see ES3 15.1.2.2 first step.

/be
Attachment #305126 - Flags: review+ → review-
I don't object to a tiny amount of code to avoid doing something expensive when it can be avoided with a quick test.

/be
(Assignee)

Comment 27

11 years ago
Created attachment 307392 [details] [diff] [review]
updated

adds the radix == 10 check (was hoping to do better, but it doesn't seem easy or obvious) and removes the "value" var from previous patch.
Attachment #305126 - Attachment is obsolete: true
Attachment #307392 - Flags: review?(brendan)
Comment on attachment 307392 [details] [diff] [review]
updated

JS_SET_RVAL is more for the outside world, and indeed you see *vp = ... used in context to set the r.v., so best to do likewise at this point. Thanks,

/be
Attachment #307392 - Flags: review?(brendan) → review+
(Assignee)

Comment 29

11 years ago
yeah, I missed radix == 0, too, new patch in a second.
(Assignee)

Comment 30

11 years ago
Created attachment 307404 [details] [diff] [review]
add radix == 0 (equivalent to 10) check, and pick nit for Brendan
Attachment #307392 - Attachment is obsolete: true
Attachment #307404 - Flags: review?(brendan)
Comment on attachment 307404 [details] [diff] [review]
add radix == 0 (equivalent to 10) check, and pick nit for Brendan

Oops, how could I forget 0? Reorder the tests to test for 0 first, since it is by far the most common (the default, just visible at the top of context). Thanks,

/be
Attachment #307404 - Flags: review?(brendan) → review+
Attachment #307392 - Flags: review+
(Assignee)

Comment 32

11 years ago
Created attachment 307406 [details] [diff] [review]
one more swing
Attachment #307404 - Attachment is obsolete: true
Attachment #307406 - Flags: review?(brendan)
Comment on attachment 307406 [details] [diff] [review]
one more swing

I marked r+ with request to make a small change, so no need to re-request review!

/be
Attachment #307406 - Flags: review?(brendan) → review+
(Assignee)

Updated

11 years ago
Attachment #307406 - Flags: approval1.9?

Comment 34

11 years ago
Comment on attachment 307406 [details] [diff] [review]
one more swing

You are blocking+ - clear to land without patch approval
Attachment #307406 - Flags: approval1.9?
(Assignee)

Comment 35

11 years ago
jsnum.c: 3.111
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: Optimize parseInt for Number → Optimize parseInt for integers in base-10

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.