Closed Bug 556830 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: perf, Whiteboard: fixed-in-tracemonkey)

Attachments

(6 files)

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
Attached file testcase
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
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.
Attached patch Proposed patchSplinter Review
Not sure how to add a test for this....
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attachment #436746 - Flags: review?(jorendorff)
Attachment #436746 - Flags: review?(jorendorff) → review+
Comment on attachment 436746 [details] [diff] [review]
Proposed patch

>From: Boris Zbarsky <bzbarsky@mit.edu>
>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
OK, made those changes.
Pushed http://hg.mozilla.org/tracemonkey/rev/698ace1f1027
Flags: in-testsuite?
Whiteboard: [fixed-in-tracemonkey]
Whiteboard: [fixed-in-tracemonkey] → fixed-in-tracemonkey
looks like this patch has resulted in a jsreftest failure for the last day or so.
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....
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...
Attached file Shell testcase
Attached file Browser testcase
And indeed, this seems to fail in browser but not in shell...
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
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)) {
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?
I backed this out for now to get the tree green while we discuss option:
  http://hg.mozilla.org/tracemonkey/rev/0fc19b242947
(In reply to comment #16)
> Would it make sense to have js_CheckUndeclaredVarAssignment treat BINDNAME as
> SETNAME?

Yes.

/be
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.
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.
Yeah, we should not be relying on fast expandos in chrome code (which, I believe, has strict mode turned on by default).
Whiteboard: fixed-in-tracemonkey
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.
This would slide under the patch that actually fixes this bug.
Attachment #438180 - Flags: review?(jorendorff)
Keywords: perf
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.
Attachment #438180 - Flags: review?(jorendorff) → review+
> 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....
(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
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?).
Yes, bits is bits. We could split constants or try disjoint enums and see how that looks, but in a separate bug.

/be
http://hg.mozilla.org/mozilla-central/rev/698ace1f1027
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
oops, this got backed out. but we should land the new patch!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
blocking2.0: --- → final+
Pushed the resolve changes and the original patch again:
  http://hg.mozilla.org/tracemonkey/rev/354aba9ace78
  http://hg.mozilla.org/tracemonkey/rev/259971053e0b
Whiteboard: fixed-in-tracemonkey
And pushed http://hg.mozilla.org/tracemonkey/rev/15091841f59e to fix debug orange.
http://hg.mozilla.org/mozilla-central/rev/354aba9ace78
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.