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?

Updated

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

Updated

12 years ago
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

11 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.

Updated

11 years ago
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+

Updated

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