bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

match_or_replace improvements

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Robert Sayre, Assigned: mrbkap)

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
unpack-code spends more than 50% of its time in match_or_replace.

Comment 1

11 years ago
Per Shark, for match_or_replace:

30.6% ComputeGlobalThis
24.5% js_Interpret (7.4% JS_GC, 7.2% InternNonIntElementId, 3.6% js_GetProperty)
20.9% js_ExecuteRegExp (10.4% MatchRegExp, 7.9% JS Arena maintenance)
 5.3% js_NewStringCopyN
 4.0% JS_realloc
 3.0% __memcpy
How old's that tree?  Surprised to see CGT that high.
(Reporter)

Comment 3

11 years ago
Yes, that should be tried again today.

Comment 4

11 years ago
That was with a build from after the recent CGT changes. I was equally surprised.

Comment 5

11 years ago
So jQuery uses the packer from http://dean.edwards.name/packer/ and it looks like the others do too. For the simple |var foo = 1;| with Base62 encoding the following is generated (prettified by me):

eval(
  function(p,a,c,k,e,r) {
    e = String;
    if (!''.replace(/^/, String)) {
      while (c--)
        r[c] = k[c] || c;
      k = [function(e) { return r[e] }];
      e = function() {
        return '\\w+';
      }
      c = 1;
    }

    while (c--) {
      if (k[c])
        p = p.replace(new RegExp('\\b' + e(c) + '\\b', 'g'), k[c]);
    }
    return p;
  }
  ('0 2=1;', 3, 3, 'var||foo'.split('|'), 0, {})
);


So it has an array of words, and each word in the original code is replaced by a number or letter (0-9a-zA-Z). It then either uses a look-up function as the argument to replace() (needing only one call), or it iterates over the array of words manually and calls replace for each (index,word) pair.

Note that for more than 10 words the generated script uses (instead of |e = String|):

e = function(c) {
  return c.toString(a);
}

to convert from base 10 to base 62 (well, base N, really, with N passed in as the |a| argument).
Indeed, we're hitting CGT from the do_push_rval case with op == JSOP_CALLNAME.  (Tree from this morning plus PCT-fix and arrays.)

Digging some more.
(Reporter)

Comment 7

11 years ago
<shaver>	ah. they eval
<shaver>	eval == heavyweight
<shaver>	heavyweight == call object
<shaver>	call == CGT

Comment 8

11 years ago
Does that explain this tree?

str_replace
  match_or_replace
    replace_glob
      find_replen
        js_Invoke
          ComputeGlobalThis
I think so, since the scripted caller is heavyweight, but I'd need to trace it more to be sure.  Brendan probably knows off the top of his head.
(Assignee)

Comment 10

11 years ago
Created attachment 304069 [details] [diff] [review]
Untested patch

It seems to me that we can skip the the js_ComputeThis in js_Invoke if !native or (fun->flags & JSFUN_FAST_NATIVE): any scripted callee will go through JSOP_THIS or kin and any fast native has to use JS_THIS{,_OBJECT} anyway. I'm not set up to test this right now, though.
(Assignee)

Updated

11 years ago
Attachment #304069 - Attachment description: Unteseted patch → Untested patch
(Assignee)

Comment 11

11 years ago
Created attachment 304101 [details] [diff] [review]
Handle non-callable things

I haven't done any perf testing on the testcase here, but this should avoid the pain that jag was seeing (at least based on what I can glean out of that packed JS function!).
Assignee: crowder → mrbkap
Status: NEW → ASSIGNED
Attachment #304101 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #304069 - Attachment is obsolete: true
Comment on attachment 304101 [details] [diff] [review]
Handle non-callable things

>Index: jsinterp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
>retrieving revision 3.432
>diff -p -U8 -r3.432 jsinterp.c
>--- jsinterp.c	18 Feb 2008 21:01:47 -0000	3.432
>+++ jsinterp.c	18 Feb 2008 22:47:54 -0000
>@@ -1256,17 +1256,18 @@ have_fun:
>     if (flags & JSINVOKE_CONSTRUCT) {
>         JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1]));
>     } else {
>         /*
>          * We must call js_ComputeThis in case we are not called from the
>          * interpreter, where a prior bytecode has computed an appropriate
>          * |this| already.
>          */
>-        if (!js_ComputeThis(cx, JS_FALSE, vp + 2)) {
>+        if (native && fun && !(fun->flags & JSFUN_FAST_NATIVE) &&
>+            !js_ComputeThis(cx, JS_FALSE, vp + 2)) {

Comment wants an update to describe the only-slow-native logic.  Perhaps merge slow-native
guard into else if, leaving js_ComputeThis call distinct to follow comment?

You'd think that after 12 years I'd have a better sense of what style should prevail.
string-unpack-code in the browser is very glad to make your patch's acquaintance.
Summary: match_and_replace improvements → match_or_replace improvements
(Assignee)

Comment 14

11 years ago
Created attachment 304138 [details] [diff] [review]
Updated comment
Attachment #304101 - Attachment is obsolete: true
Attachment #304138 - Flags: review?(brendan)
Attachment #304101 - Flags: review?(brendan)
Comment on attachment 304138 [details] [diff] [review]
Updated comment

r/a=me with "But we need to compute |this| eagerly only for slow native functions."

/be
Attachment #304138 - Flags: review?(brendan)
Attachment #304138 - Flags: review+
Attachment #304138 - Flags: approval1.9+
(Assignee)

Comment 16

11 years ago
Created attachment 304142 [details] [diff] [review]
With that
(Assignee)

Comment 17

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 18

11 years ago
Nice! Went from 760ms to 510ms here.

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.