Closed
Bug 324161
Opened 19 years ago
Closed 17 years ago
Optimize parseInt for integers in base-10
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: erik, Assigned: crowderbt)
References
Details
(Keywords: perf, testcase)
Attachments
(4 files, 7 obsolete files)
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•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Maybe the same thing should be done for Rhino as well?
Reporter | ||
Comment 3•19 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•19 years ago
|
||
Attachment #209121 -
Attachment is obsolete: true
Comment 5•19 years ago
|
||
Math.floor and |0 don't do the same thing. Consider negative numbers.
Comment 6•19 years ago
|
||
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 7•19 years ago
|
||
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•19 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.
Attachment #209177 -
Flags: superreview?(shaver)
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.
Comment 10•18 years ago
|
||
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
Comment 11•18 years ago
|
||
gprof2html can convert this profile data to html view
Comment 12•18 years ago
|
||
Why not just attach the HTML?
Comment 13•18 years ago
|
||
(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
Comment 14•18 years ago
|
||
Comment 15•18 years ago
|
||
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.
Attachment #245798 -
Attachment description: Various result win32 → Various results win32
Comment 16•18 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>
Attachment #246515 -
Attachment is obsolete: true
Updated•17 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 18•17 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•17 years ago
|
||
SunSpider results coming...
Attachment #209177 -
Attachment is obsolete: true
Assignee | ||
Comment 20•17 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•17 years ago
|
Flags: tracking1.9+ → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 21•17 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•17 years ago
|
||
And yes, I realize that comment 18 is me being a silly person.
Comment 23•17 years ago
|
||
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•17 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 25•17 years ago
|
||
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-
Comment 26•17 years ago
|
||
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•17 years ago
|
||
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 28•17 years ago
|
||
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•17 years ago
|
||
yeah, I missed radix == 0, too, new patch in a second.
Assignee | ||
Comment 30•17 years ago
|
||
Attachment #307392 -
Attachment is obsolete: true
Attachment #307404 -
Flags: review?(brendan)
Comment 31•17 years ago
|
||
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•17 years ago
|
Attachment #307392 -
Flags: review+
Assignee | ||
Comment 32•17 years ago
|
||
Attachment #307404 -
Attachment is obsolete: true
Attachment #307406 -
Flags: review?(brendan)
Comment 33•17 years ago
|
||
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•17 years ago
|
Attachment #307406 -
Flags: approval1.9?
Comment 34•17 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•17 years ago
|
||
jsnum.c: 3.111
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: Optimize parseInt for Number → Optimize parseInt for integers in base-10
Updated•17 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•