Closed Bug 324161 Opened 19 years ago Closed 17 years ago

Optimize parseInt for integers in base-10

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: erik, Assigned: crowderbt)

References

Details

(Keywords: perf, testcase)

Attachments

(4 files, 7 obsolete files)

Also tests bitwise or
554 bytes, text/html
Details
gprofile output for this testcase
80.70 KB, application/x-gzip
Details
HTML output from grpof2html
174.16 KB, application/x-gzip
Details
one more swing
905 bytes, patch
brendan
: review+
Details | Diff | Splinter Review
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.
Attached file Test case (obsolete) —
Maybe the same thing should be done for Rhino as well?
Keywords: testcase
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?
Attached file Also tests bitwise or —
Attachment #209121 - Attachment is obsolete: true
Math.floor and |0 don't do the same thing. Consider negative numbers.
Attached patch possible fix (obsolete) — — Splinter Review
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+
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)
Keywords: perf
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
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
Attached file Various results win32 (obsolete) —
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 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>
Attached file Various results win32 (obsolete) —
oops,sorry for bugspam.
Attachment #245798 - Attachment is obsolete: true
Blocks: 117611
Attachment #246515 - Attachment is obsolete: true
Flags: blocking1.9?
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+
Attached patch Update to current trunk (obsolete) — — Splinter Review
SunSpider results coming...
Attachment #209177 - Attachment is obsolete: true
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
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)
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+
(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
Attached patch updated (obsolete) — — Splinter Review
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+
yeah, I missed radix == 0, too, new patch in a second.
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+
Attached patch one more swing — — Splinter Review
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+
Attachment #307406 - Flags: approval1.9?
Comment on attachment 307406 [details] [diff] [review] one more swing You are blocking+ - clear to land without patch approval
Attachment #307406 - Flags: approval1.9?
jsnum.c: 3.111
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: Optimize parseInt for Number → Optimize parseInt for integers in base-10
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: