Closed Bug 324161 Opened 19 years ago Closed 16 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.
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: 16 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: