503694 - TM: Code run off an event handler is never traced

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dorus Peelen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090701 Minefield/3.6a1pre

Starting with Gecko/20090701 Minefield/3.6a1pre, enabling or disabling javascript.options.jit.content no longer result in a performance difference on Greg Reimer's loop benchmark test (see url).

Previous enabling JIT resulted in a significant speed up.

To be clear:
Works in: Gecko/20090630 Minefield/3.6a1pre
Broke in: Gecko/20090701 Minefield/3.6a1pre

Reproducible: Always

Comment 1

[Kevin Brosnan]

I get slow script warnings in the current trunk. Using 20090629 I get no such warnings. In my current trunk build I get forEach ~206000ms.

Comment 2

[Dorus Peelen]

The slow script warning has is probably to blame for that 206000ms. Increase the value in dom.max_script_run_time to prevent that.
However, that you get the no script warning in the first place does indicate current trunk is slower, it's about twice as slow as it used to be by my calculations. (well, its as slow as it previous was with jit disabled).

Comment 3

[Brendan Eich [:brendan]]

Need to confirm and find the regressing changeset.

/be

Comment 4

[Kevin Brosnan]

Attachment: Excel file showing the performance difference of several tests

Did three runs of the test for 2009-06-30 and 2009-07-01 averaged the results and calculated a simple percentage change, positive is slower, negative is faster.

Many of the tests are multiples of 100% slower.

Changesets involved http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1b2f9a366ca&tochange=d54a92a63aa7

All of the above was tested in a new profile. Something is wrong with one of my profiles. Even in safe mode I get dreadful performance on one of the tests hence my comment 1.

Comment 5

[Kevin Brosnan]

Filed Bug 504271 for my issue in comment 1. javascript.options.strict is the cause

Comment 6

[Mike Shaver (:shaver emeritus)]

Safe mode disables the JIT.  Can you export that Excel file as CSV or text or even a screenshot? Many of us (including myself on this computer) don't have a way to view Excel files usefully.

Comment 7

[IU]

Attachment: Screenshot of benchmarks

Comment 8

[Mike Shaver (:shaver emeritus)]

Great, thanks.  Would be excellent if you could use ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009-06-30-03-tracemonkey/ and friends to narrow down the regression range on tracemonkey, which will point to a much smaller set of changes.  Up for it?

Comment 9

[Kevin Brosnan]

Attachment: CSV file showing the performance difference of several tests

There were three worksheets, merged to one csv.

Comment 10

[Dorus Peelen]

I try to friend a narrow regression range, unless one of you is doing so already, give a yell please :)

Comment 11

[Dorus Peelen]

I got it down to:

2009-06-26-03-tracemonkey works
2009-06-27-03-tracemonkey slow

Comment 12

[Boris Zbarsky [:bzbarsky]]

Dorus, thanks for hunting that regression range down.  This looks like a regression from bug 500580.  The test in this bug runs its code like so:

  <a href="#" onclick="this.style.display='none';test();return false;">test</a>

That means that when we invoke the handler the scope chain starts at the <a> node, not the global object.  And the fix for bug 500580 then disables the jit for the duration of the handler execution.

Here's a simple testcase that can be used to test this:

  data:text/html,<script>function f() { var s = new Date; for (var i = 0; i < 1e7; ++i); alert(new Date-s);} f()</script><button onclick="f()">click</button>

Load that URI, get the first alert, click the button, get the second alert, compare the two numbers.  In 1.9.1, they're about equal.  On m-c and t-m they're off by an order of magnitude, as expected for jit off....

Comment 13

[Brendan Eich [:brendan]]

Argh, the fix for bug 500580, and the fix for bug 497060 before it, is too pessimistic. Static scope rules in JS mean that even though onclick="f()" creates a function with a deep scope chain, the call to f is not deep.

Note also that if there were some way to make the <script> in which f is defined have a deep scope chain, but f called function g defined in a script with a shallow (global object only) scope chain, then the fix for bug 497060 would bite in the same fashion.

We really want JIT disabling to be statically scoped, not dynamically scoped as in the fixes for those bugs. Graydon, if you take this I will do a better job advising.

/be

Comment 14

[Graydon Hoare :graydon]

Sure. I figured this was on my plate sooner or later. I think I already might have one about it open still. Plus, the existing one is still making bc's test rig cry. It was a hack to get it to "safe", need to fix right.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

This seems like a P1 for the mozilla-1.9.2 branch; out of curiousity, are we not tracking a sunspider score on our nightly trunk builds that should have alerted us to this sooner?

Comment 16

[Mike Shaver (:shaver emeritus)]

Sunspider doesn't run off events, this is another test that isn't in our suite (and is a little rough around the edges to incorporate, I think).

Comment 17

[Benjamin Smedberg]

Dromaeo does, though, right?

Comment 18

[Mike Shaver (:shaver emeritus)]

No, I believe Dromaeo uses setTimeout.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> (and is a little rough around the edges to incorporate, I think).

I we had a performance suite we could just drop tests into, writing a test for this would be super-easy....

Comment 20

[Robert Sayre]

(In reply to comment #19)
> > (and is a little rough around the edges to incorporate, I think).
> 
> I we had a performance suite we could just drop tests into, writing a test for
> this would be super-easy....

http://hg.mozilla.org/users/rsayre_mozilla.com/benchmarker/

Comment 21

[Graydon Hoare :graydon]

Attachment: Rough cut at fixing this the "more correct" way.

I talked to brendan a bit today and worked out, I think, an approach that should cost no more than the current code but free us from the blunt instrument of disabling the JIT altogether when activated on an event handler.

Haven't done performance comparisons yet though. Just posting here for initial feedback. It certainly *works*, and I've checked that it bails on loop headers that are embedded in event handlers; I'll toss it at the try server tonight as well to see how it fares.

One question though: should we blacklist if we keep trying to enter (and bailing) this way? It seems like it'd be rare for there to be a loop header literally *in* an event handler body (rather than reachable-through it) but that case will repeatedly try-to-enter and fail here. Very cheaply, but still.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 392832 [details] [diff] [review]
Rough cut at fixing this the "more correct" way.


>+
>+/*
>+ * We cache name lookup results only for the global object or for native
>+ * non-global objects without prototype or with prototype that never mutates,
>+ * see bug 462734 and bug 487039.
>+ */
>+

Nit: no blank line here, move it down...

>+extern JS_FRIEND_DATA(JSClass) js_CallClass;
>+extern JS_FRIEND_DATA(JSClass) js_DeclEnvClass;

... to here. Or could you move the externs into the static inline for narrowest scope?

>+    /*
>+     * The JIT records and exepects to execute with two scope-chain

tpyo

>+     * assumptions baked-in:
>+     *
>+     *   1. That the bottom of the scope chain is global, in the sense of
>+     *      JSCLASS_IS_GLOBAL.
>+     *
>+     *   2. That the scope chain between fp and the global is free of
>+     *      "unusual" native objects such as HTML forms or other funny
>+     *      things.
>+     *
>+     * #2 is checked here while following the scope-chain links, via
>+     * js_IsCacheableNonGlobalScope, which consults a whitelist of known
>+     * class types; once a global is found, it's checked for #1. Failing
>+     * either check causes an early return from execution.
>+     */
>+

Nit: no blank line needed here.

>+    JSObject* globalObj;
>+    JSObject *child = cx->fp->scopeChain;

Nit: T* p vs T *p conflict -- jstracer.cpp style favors T* p.

>+    while ((globalObj = OBJ_GET_PARENT(cx, child)) != NULL) {
>+        if (!js_IsCacheableNonGlobalScope(child))
>+            return NULL;
>+        child = globalObj;
>+    }
>+    globalObj = child;

Might be cleaner and use fewer lines to s/globalObj/parent/g and then s/child/globalObj/g, although this has the odd-looking effect of calling non-globals along the scope chain "globalObj" temporarily.

To avoid that, how about using JSObject* parent instead of globalObj until the last line cited above, at which point introduce JSObject* globalObj = child; ? Cleanest looking and the compiler can figure out minimal register allocation :-).

>+    if (!(OBJ_GET_CLASS(cx, globalObj)->flags & JSCLASS_IS_GLOBAL))
>+        return NULL;

Otherwise looks great! r=me with nits picked.

/be

Comment 23

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/f5fd44e7cfdb

(With blacklisting, as suggested on IRC. Appears to satisfy the try servers and improves the URL case listed in this bug. Not sure if it entirely fixes the world, but i

Comment 24

[Graydon Hoare :graydon]

Oh, weird. Cut off. "but it doesn't seem to make anything I can find any worse."

Comment 25

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f5fd44e7cfdb

Comment 26

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2fbdb5c62928