556830 - Lots of property cache misses on free variable access in function created by eval

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current t-m tip js shell

STEPS TO REPRODUCE:
1)  Turn on propcache metering.
2)  Run the attached testcase in the shell _without_ -j

EXPECTED RESULTS: hit rates north of 90%

ACTUAL RESULTS: hit rates around 50%

This is NOT bug 489098, since there are no disfills.  But if I just create the function directly instead of via eval, I get lots of hits.  If I remove the wrapper "testme" function I also get lots of hits.

This seems to account for a lot of our performance cost at http://herberthamaral.com/metaprog/index.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

The relevant nonzero stats:

      fills          4
   modfills          3
 brandfills          2
      tests        203
     pchits         99
   settests        100
  setpchits         99
  setmisses          1
  kpcmisses        104
     misses        104
   pcpurges          3

Comment 3

[Boris Zbarsky [:bzbarsky]]

Talked to Jason.  The issue is that our bindname never fills the propcache, so we miss all the bindname lookups.  And the reason it doesn't fill is this code in js_FindIdentifierBase:

        /* Call and other cacheable objects always have a parent. */
        obj = obj->getParent();
        if (!obj->getParent())
            return obj;

in out case, scopeChain is a Call whose parent is the global the prop we want is on the global, so we don't fill the cache because we take this early exit.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Not sure how to add a test for this....

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 436746 [details] [diff] [review]
Proposed patch

>From: Boris Zbarsky <[e-mail]>
>Bug 556830. Fill the propcache even if we start the lookup on a Call whose parent is the global the property is on.  r=jorendorff
>
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -4833,41 +4833,48 @@ js_FindIdentifierBase(JSContext *cx, JSO
>     JSObject *obj = scopeChain;
> 
>     /*
>      * Loop over cacheable objects on the scope chain until we find a
>      * property. We also stop when we reach the global object skipping any
>      * farther checks or lookups. For details see the JSOP_BINDNAME case of
>      * js_Interpret.
>      */
>-    for (int scopeIndex = 0; js_IsCacheableNonGlobalScope(obj); scopeIndex++) {
>+    for (int scopeIndex = 0;
>+         /*
>+          * The test order here matters; js_IsCacheableNonGlobalScope
>+          * should only get passed things with parents.
>+          */
>+         !obj->getParent() || js_IsCacheableNonGlobalScope(obj);
>+         scopeIndex++) {

Drive-by super-nits:

The new comment might be better as a second paragraph in the existing major comment before the for keyword.

The ; in the new comment should be a : or even a " because".

The "should only get passed things with parents" should be "must not be passed a global object (i.e., one with null parent)".

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, made those changes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/698ace1f1027

Comment 8

[Robert Sayre]

looks like this patch has resulted in a jsreftest failure for the last day or so.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ugh.  Indeed.  I'm going to try to reproduce in jsreftest locally tomorrow morning; so far my attempt to just run the testcase "by hand" by copy/pasting the relevant code makes the testcase pass for me....

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  Looks like the failing test is ecma_5/strict/8.7.2.js, and in particular this chunk:

assertEq(testLenientAndStrict('undeclared=1',
                              completesNormally,
                              raisesException(ReferenceError)),
         true);

Or at least when I comment out that hunk the test passes.  But if I extract the relevant code into a shell testcase it passes for me.  Maybe the fail is browser-only...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Shell testcase

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Browser testcase

And indeed, this seems to fail in browser but not in shell...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal-ish failing browser testcase

Comment 14

[Boris Zbarsky [:bzbarsky]]

Here's the stack showing the problem:

#0  js_DefineProperty (cx=0x1465600, obj=0x16110760, id=373450980, value=22, getter=0
x344b28 <JS_PropertyStub>, setter=0x344b28 <JS_PropertyStub>, attrs=1) at /Users/bzba
rsky/mozilla/tracemonkey/mozilla/js/src/jsobj.cpp:4362
#1  0x0035e2a0 in JSObject::defineProperty (this=0x16110760, cx=0x1465600, id=3734509
80, value=22, getter=0x344b28 <JS_PropertyStub>, setter=0x344b28 <JS_PropertyStub>, a
ttrs=1) at jsobj.h:459
#2  0x0034dd5c in DefineUCProperty (cx=0x1465600, obj=0x16110760, name=0x1687e810, na
melen=10, value=22, getter=0x344b28 <JS_PropertyStub>, setter=0x344b28 <JS_PropertySt
ub>, attrs=1, flags=0, tinyid=0) at /Users/bzbarsky/mozilla/tracemonkey/mozilla/js/sr
c/jsapi.cpp:2903
#3  0x0034deb3 in JS_DefineUCProperty (cx=0x1465600, obj=0x16110760, name=0x1687e810,
 namelen=10, value=22, getter=0x344b28 <JS_PropertyStub>, setter=0x344b28 <JS_Propert
yStub>, attrs=1) at /Users/bzbarsky/mozilla/tracemonkey/mozilla/js/src/jsapi.cpp:3523
#4  0x12bb87bd in nsWindowSH::NewResolve (this=0x1ef191c0, wrapper=0x16e2f120, cx=0x1
465600, obj=0x16110760, id=373450980, flags=2, objp=0xbfffaf28, _retval=0xbfffaf2c) a
t /Users/bzbarsky/mozilla/tracemonkey/mozilla/dom/base/nsDOMClassInfo.cpp:6777
#5  0x1187b666 in XPC_WN_Helper_NewResolve (cx=0x1465600, obj=0x16110760, idval=37345
0980, flags=2, objp=0xbfffafb4) at /Users/bzbarsky/mozilla/tracemonkey/mozilla/js/src
/xpconnect/src/xpcwrappednativejsops.cpp:1221
#6  0x003ef784 in js_LookupPropertyWithFlags (cx=0x1465600, obj=0x16110760, id=373450
980, flags=2, objp=0xbfffb034, propp=0xbfffb030) at /Users/bzbarsky/mozilla/tracemonk
ey/mozilla/js/src/jsobj.cpp:4612
#7  0x003efd61 in js_FindIdentifierBase (cx=0x1465600, scopeChain=0x1610a300, id=3734
50980) at /Users/bzbarsky/mozilla/tracemonkey/mozilla/js/src/jsobj.cpp:4851

Comment 15

[Boris Zbarsky [:bzbarsky]]

The relevant nsWindowSH code is:

6777          if (!::js_CheckUndeclaredVarAssignment(cx) ||
6778              !::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),
6779                                     ::JS_GetStringLength(str), JSVAL_VOID,
6780                                     JS_PropertyStub, JS_PropertyStub,
6781                                     JSPROP_ENUMERATE)) {

Comment 16

[Boris Zbarsky [:bzbarsky]]

So the issue is that ::js_CheckUndeclaredVarAssignment always returns true if the current jsop is not SETNAME.  And in the stack above it's BINDNAME, of course.

Would it make sense to have js_CheckUndeclaredVarAssignment treat BINDNAME as SETNAME?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I backed this out for now to get the tree green while we discuss option:
  http://hg.mozilla.org/tracemonkey/rev/0fc19b242947

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Would it make sense to have js_CheckUndeclaredVarAssignment treat BINDNAME as
> SETNAME?

Yes.

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

Jason's opposed to that. 

I just spent a while digging through this stuff and this seems to be the story:

1)  The patch in this bug basically backs out bug 462734.  That's ok because
    propcache is now tied to pc so bind and set can't collide.
2)  The patch in bug 488285 is what switched the classinfo code from just
    defining on BINDNAME to the current setup, where
    js_CheckUndeclaredVarAssignment always returns true during BINDNAME (but
    there used to be no lookup during BINDNAME due to how bug 462734 was fixed).
3)  The lookup during bindname is needed for correctness, but only in strict
    mode.  See bug 462734 comment 14.  Jason's suggested testcase for that is
    something like:
 
      "use strict";
      x = (function() { this.x = 5; return 6; })()

    which should throw but doesn't in our code.

Jason is opposed to comment 16's proposal.  What we're thinking right now is that we'll change the classinfo code to not define the prop if in strict mode.  Then in non-strict mode we can avoid doing the resolve in js_FindPropertyBase.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The main drawback will be that if our internal strict mode is enabled the optimization in classinfo will be disabled.  I think that's ok.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, we should not be relying on fast expandos in chrome code (which, I believe, has strict mode turned on by default).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Thought about this more, and we do want the fast expandos in debug builds (since TMFLAGS doesn't work in opt builds), so I need to log the javascript.options.strict warnings from classinfo....  I'll try to get that written up tomorrow.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix for that test issue

This would slide under the patch that actually fixes this bug.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 438180 [details] [diff] [review]
Proposed fix for that test issue

Great. defineHow is already a bitset, so please add a JSDNP_UNQUALIFIED bit instead of adding an argument to js_SetPropertyHelper.

+        if (!obj->getParent() && unqualified &&
+            !js_CheckUndeclaredVarAssignment(cx, ID_TO_VALUE(id))) {
+            return JS_FALSE;
+        }

The preferred style in js/src is to put each operand of && on its own line when they're too long to fit together on a single line:

+        if (!obj->getParent() &&
+            unqualified &&
+            !js_CheckUndeclaredVarAssignment(cx, ID_TO_VALUE(id))) {
+            return JS_FALSE;
+        }

r=me with those changes. But please run SunSpider and V8 benchmarks before checking in to make sure this doesn't hurt anything.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> so please add a JSDNP_UNQUALIFIED bit

That doesn't make sense as a flag to js_DefineNativeProperty (which is what the JSDNP_* bits are), though....

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> > so please add a JSDNP_UNQUALIFIED bit
> 
> That doesn't make sense as a flag to js_DefineNativeProperty (which is what the
> JSDNP_* bits are), though....

We use same bits for js_SetPropertyHelper, because setting can "define" (craete) a property too.

/be

Comment 27

[Boris Zbarsky [:bzbarsky]]

Yes, I understand that.  The question is whether it's ok to add a flag here that's completely meaningless for js_DefineNativeProperty (and presumably document it as such?).

Comment 28

[Brendan Eich [:brendan]]

Yes, bits is bits. We could split constants or try disjoint enums and see how that looks, but in a separate bug.

/be

Comment 29

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/698ace1f1027

Comment 30

[Robert Sayre]

oops, this got backed out. but we should land the new patch!

Comment 31

[Boris Zbarsky [:bzbarsky]]

New patch still needs changes and test runs from comment 24 and I'm still on leave.  Not likely to happen for a bit here, therefore (esp. since meaningful test runs are a pain due to having to shut everything off first).

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed the resolve changes and the original patch again:
  http://hg.mozilla.org/tracemonkey/rev/354aba9ace78
  http://hg.mozilla.org/tracemonkey/rev/259971053e0b

Comment 33

[Boris Zbarsky [:bzbarsky]]

And pushed http://hg.mozilla.org/tracemonkey/rev/15091841f59e to fix debug orange.

Comment 34

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/354aba9ace78