115627 - Performance inefficiency in js_strtod

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[tenthumbs]

This fell out of some research into mozilla malloc usage. If you run 
"mozilla about:blank" on Linux then js_strtod makes 469 malloc calls 
with requests ordered like this:

 size  requests
   1       1
   2     335
   3      93
   4      21
   5      11
   6       6
   7       1
  10       1

Lots of action for very little. I think that using a small local buffer 
would help performance, 8 bytes would eliminate all but one call.

Now I haven't the foggiest idea how JS really works so I don't know if 
this is important or not but I thought you'd like to know. :-)

Comment 1

[Phil Schwartau]

Reassigning to Kenton; cc'ing Brendan in case this is of interest - 

Comment 2

[tenthumbs]

Here's a completely untested idea but it shows what I mean.

--- jsnum.c.orig        Tue Nov  6 14:37:06 2001
+++ jsnum.c     Mon Dec 17 13:07:34 2001
@@ -789,14 +789,19 @@
 {
     size_t i;
     char *cstr, *istr, *estr;
+    char cbuf[8];
     JSBool negative;
     jsdouble d;
     const jschar *s1 = js_SkipWhiteSpace(s);
     size_t length = js_strlen(s1);

-    cstr = (char *) malloc(length + 1);
-    if (!cstr)
-       return JS_FALSE;
+    if (length < sizeof(cbuf))
+        cstr = cbuf
+    else {
+        cstr = (char *) malloc(length + 1);
+        if (!cstr)
+           return JS_FALSE;
+    }
     for (i = 0; i <= length; i++) {
        if (s1[i] >> 8) {
            cstr[i] = 0;
@@ -833,7 +838,8 @@
     }

     i = estr - cstr;
-    free(cstr);
+    if (cstr != cbuf)
+        free(cstr);
     *ep = i ? s1 + i : s;
     *dp = d;
     return JS_TRUE;

Comment 3

[Jo Hermans]

This sounds like a good and safe idea. But I would make the array 12 bytes, not
8. This leaves enough room for the largest 32bit number (10 bytes), a possible
negative sign, and the \0.

Comment 4

[Bernard Alleysson]

I vote for 12 bytes
for instance, I think that js_strtod() (thus unecessary malloc()/free()) is
called for every numeric constant in a js script file

Comment 5

[Bernard Alleysson]

Attachment: Use stack buffer to avoid malloc

patch after tenthumbs idea

Comment 6

[Brendan Eich [:brendan]]

Great -- thanks, tenthumbs and balleysson.  I'm cc'ing potential reviewers and
I'll sr= in a second.  With an r= today, I'll check in too.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 63826 [details] [diff] [review]
Use stack buffer to avoid malloc

sr=[e-mail]

Comment 8

[tenthumbs]

The only question is whether a local buffer exteneds the stack too much, 
but I don't think that's important.

Comment 9

[Brendan Eich [:brendan]]

stacks are page allocated, 12 << pagesize, don't worry.  r=, anyone?
 
/be

Comment 10

[Kenton Hanson (gone)]

Comment on attachment 63826 [details] [diff] [review]
Use stack buffer to avoid malloc

r=khanson

I'd prefer to see larger buffer size.  Maybe 25 or 30, enough to uniquely
define any double precision number in decimal format.

=Kenton=

Comment 11

[Brendan Eich [:brendan]]

khanson: good point, I made cbuf 32 chars long.  Thanks all,

/be

Comment 12

[Phil Schwartau]

Marking Verified - 

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please don't use C++ comments in C files.  (AIX is red.  I'll check in the fix
if nobody else does in the near future.)

Comment 14

[Brendan Eich [:brendan]]

Argh!  I used to be the archetype of the old C hacker, but after hacking XPCOM
code (FastLoad), I'm useless at spotting C++ comments in C code.  Thanks, dbaron
and sorry for the trouble.  Bernard, take note!

/be