540985 - Regression in Peacekeeper string benchmarks due to regexp literals no longer tracing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Between the 2010-01-11 m-c build and the 2010-01-12 m-c build, the stringRegexpEmail and stringValidateForm benchmarks in peacekeeper regressed significantly (30-50%).

m-c pushlog for the range includes a t-m merge: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=120667a01fd2&tochange=a43e2f7eda8f

Bisecting now.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and steps to reproduce:

1)  hg clone http://hg.mozilla.org/users/mpalmgren_mozilla.com/peacekeeper
2)  Start a browser and point to run.html from that pull
3)  Select "stringValidateForm" or "stringRegexpEmail" in the "string" dropdown
4)  Click "run selected" next to said dropdown.

Comment 2

[Boris Zbarsky [:bzbarsky]]

bisect says:

The first bad revision is:
changeset:   36827:3862a7e48e79
user:        Brendan Eich <[e-mail]>
date:        Mon Jan 04 12:25:44 2010 -0800
summary:     ES5-conformance fix to make a new RegExp for each regexp literal evaluation (98409, r=jwalden).

In retrospect, this should not be a surprise....

Comment 3

[Boris Zbarsky [:bzbarsky]]

From that checkin's comment changes:

+         * If the regexp's script is one-shot and the regexp is not used in a
+         * loop, we can avoid the extra fork-on-exec costs of JSOP_REGEXP by
+         * selecting JSOP_OBJECT. Otherwise, to avoid incorrect proto, parent,
+         * and lastIndex sharing, select JSOP_REGEXP.

followed by this hunk:

+        if (singleton) {
+            for (JSStmtInfo *stmt = cg->topStmt; stmt; stmt = stmt->down) {
+                if (STMT_IS_LOOP(stmt)) {
+                    singleton = false;
+                    break;
+                }
+            }
+        }
+        if (singleton) {
             ok = EmitObjectOp(cx, pn->pn_objbox, JSOP_OBJECT, cg);
         } else {
             ok = EmitIndexOp(cx, JSOP_REGEXP,
                              cg->regexpList.index(pn->pn_objbox),
                              cg);
         }

But of course jstracer.cpp has:

  JS_REQUIRES_STACK AbortableRecordingStatus
  TraceRecorder::record_JSOP_REGEXP()
  {
      return ARECORD_STOP;
  }

so we no longer trace loops with regexp literals in them, I bet.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Shell testcase that used to trace and now doesn't

Comment 5

[Boris Zbarsky [:bzbarsky]]

Would making JSOP_REGEXP trace like JSOP_OBJECT does but using getRegExp() do the trick?  Or does the index calculation need to be different?  I'm not sure why these are not just calling the function that computes such indices....

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Would making JSOP_REGEXP trace like JSOP_OBJECT does but using getRegExp() do
> the trick?

Certainly not -- that won't clone. As always, you want to see what the interpreter does and either emit LIR or call a builtin to do the same:

BEGIN_CASE(JSOP_REGEXP)
    /*
     * Push a regexp object cloned from the regexp literal object mapped by the
     * bytecode at pc. ES5 finally fixed this bad old ES3 design flaw which was
     * flouted by many browser-based implementations.
     *
     * XXX This code is specific to regular expression objects.  If we need a
     * similar op for other kinds of object literals, we should push cloning
     * down under JSObjectOps and reuse code here.
     */
    index = GET_FULL_INDEX(0);
    obj = js_CloneRegExpObject(cx, script->getRegExp(index), NULL);
    if (!obj)
        goto error;
    rval = OBJECT_TO_JSVAL(obj);
    PUSH_OPND(rval);
END_CASE(JSOP_REGEXP)

> Or does the index calculation need to be different?  I'm not sure
> why these are not just calling the function that computes such indices....

Not sure. Cleanup opportunity, seems like.

Thanks for id'ing the cause of the perf hit; you're right it should have been obvious. I wasn't paying attention to the regression window in the peacekeeper bug, at least not in terms of my checkins ;-).

/be

Comment 7

[Brendan Eich [:brendan]]

This is a super-easy bug to fix. bz, dmandelin, waldo, anyone -- please take it.

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Certainly not -- that won't clone.

I was wondering why the code I was looking at wasn't cloning, in fact... but that's because I was looking at the decompiler case for JSOP_REGEXP, I think.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, the good news is that I have a patch that makes this trace.

The bad news is that while it seems to get us back to where we were on stringRegexpEmail it doesn't get us nearly there on stringValidateForm.  Still about a 30% slowdown.

Profile shows that the time is spent about 12% cloning the regexp obj and the rest of the time actually executing the regexp, all on trace.

I guess I'll attach what I have.  It still needs OOM handling, unless the insCall somehow handles that for me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Sort of a start

Someone else will need to take over this; I'm more or less out of time to spend on it...  Needs someone to figure out why stringValidateForm doesn't speed up enough.

Comment 11

[Jeff Walden [:Waldo]]

Can do, after I finish a review for Luke.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch and test

Comment 13

[Igor Bukanov]

Comment on attachment 423750 [details] [diff] [review]
Patch and test

>+JSObject * JS_FASTCALL
>+js_CloneRegExpObject(JSContext *cx, JSObject *obj)
> {
...
>-    re = (JSRegExp *) obj->getPrivate();
>-    if (re) {
>+    if (JSRegExp *re = static_cast<JSRegExp *>(obj->getPrivate())) {

The current check for re is a deadwood as any compile-time regular expression has private and the check can be removed. r+ with that fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Jeff, does that get us the perf back on both tests in your testing?  Or just one like for me?

Comment 15

[Jeff Walden [:Waldo]]

I haven't tested a build from before the change, but comparing with this and without I see clear differences in the results on both tests.  What are you seeing?

Comment 16

[Jeff Walden [:Waldo]]

The contents of those tests are sufficiently small that it's entirely plausible the loss is due to lots more regex cloning happening.

Comment 17

[Jeff Walden [:Waldo]]

Hm, you're right, only gets it back on stringRegexpEmail.  I'll push this tomorrow morning and investigate the remaining slowdown then.

Bug 543097 should also win back some time by inlining the result of some redundant work in |js_CloneRegExpObject| on trace (doesn't seem worth folding in here now given the already-reviewed patch).  Judging by the numbers in that bug for a simple function expression micro-benchmark, the win in these tests may be significant.

Comment 18

[Jeff Walden [:Waldo]]

First part (*not* fixed yet), more to come once I do some profiling:

http://hg.mozilla.org/tracemonkey/rev/6a3c15f34a50

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6a3c15f34a50

Comment 20

[nemo]

I'm a little confused. Comment #18 says *not* fixed yet?
Yet the resolved fixed follows the push of that very code?

Comment 21

[Brendan Eich [:brendan]]

One patch per bug. Anything else goes in a followup -- Waldo, please file unless there's nothing more to do, on closer inspection.

/be

Comment 22

[Timothy Nikkel (:tnikkel)]

A followup was filed for the remaining perf: bug 553342.