Property cache can skip JSClass::resolve or JSClass::addProperty hooks

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(Whiteboard: fixed-in-tracemonkey [branch patch needs review])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
Two objects that share an empty scope can have different JSClasses.  If one has stubs for the resolve and addProperty hooks, and the other has custom hooks, the property cache can cause the custom hooks to be skipped.

It looks like there are two bugs around addProperty:

1. The property cache is filled even when the object in question has a nonstub addProperty hook.

2. Objects with addProperty hooks are allowed to share scopes with other objects that use the stub.

Hard to test.  :-\
(Assignee)

Comment 1

9 years ago
Here's the test case. See bug 505798 for the test harness. (The patch there already contains this test case.)

static int g_counter;

static JSBool
CounterAdd(JSContext *cx, JSObject *obj, jsval idval, jsval *vp)
{
    g_counter++;
    return JS_TRUE;
}

static JSClass CounterClass = {
    "Counter",  /* name */
    0,  /* flags */
    CounterAdd, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
    JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub,
    JSCLASS_NO_OPTIONAL_MEMBERS
};

BEGIN_TEST(addProperty)
{
    g_counter = 0;
    eval("var x = {};");
    if (!JS_DefineObject(cx, global, "y", &CounterClass, NULL, JSPROP_ENUMERATE))
        throwError();
    eval("var arr = [x, y];\n"
         "for (var i = 0; i < arr.length; i++)\n"
         "    arr[i].p = 1;\n");
    assertEq(g_counter, 1);
}
END_TEST(addProperty)

Comment 2

9 years ago
Created attachment 396574 [details] [diff] [review]
patch
Assignee: general → gal

Comment 3

9 years ago
Without the patch:

Own Scope : 45781
Not Own Scope : 110287
Not Native Scope: 69511
Total : 225579

With the patch:

Own Scope : 45782
Not Own Scope: 110288
Not Native Scope: 69511
Total : 225581

Updated

9 years ago
Attachment #396574 - Flags: review?(jorendorff)
(Assignee)

Comment 4

9 years ago
Comment on attachment 396574 [details] [diff] [review]
patch

Beautiful.

Please also commit this, since your patch makes the test pass:

diff --git a/js/src/jsapi-tests/testPropCache.cpp b/js/src/jsapi-tests/testPropCache.cpp
--- a/js/src/jsapi-tests/testPropCache.cpp
+++ b/js/src/jsapi-tests/testPropCache.cpp
@@ -12,21 +12,20 @@ CounterAdd(JSContext *cx, JSObject *obj,
 static JSClass CounterClass = {
     "Counter",  /* name */
     0,  /* flags */
     CounterAdd, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
     JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub,
     JSCLASS_NO_OPTIONAL_MEMBERS
 };
 
-BEGIN_TEST(testPropCache_bug505798)
+BEGIN_TEST(testPropCache_bug505523)
 {
     g_counter = 0;
     EXEC("var x = {};");
     CHECK(JS_DefineObject(cx, global, "y", &CounterClass, NULL, JSPROP_ENUMERATE));
     EXEC("var arr = [x, y];\n"
          "for (var i = 0; i < arr.length; i++)\n"
          "    arr[i].p = 1;\n");
-    knownFail = true;
     CHECK(g_counter == 1);
     return true;
 }
-END_TEST(testPropCache_bug505798)
+END_TEST(testPropCache_bug505523)
Attachment #396574 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 5

9 years ago
Note that this fixes only the *second* bug described in comment 0.

The first bug is still there but it's something different, so I'll file a separate bug.

Comment 6

9 years ago
http://hg.mozilla.org/tracemonkey/rev/af649bce14da
Flags: blocking1.9.2?
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+
XPConnect relies on the conditions in js_ObjectIsSimilarToProto to make all wrappers with the same proto share a scope. Do we know what the effect is of this change in XPConnect, seems like we'd create a whole lot more scopes?
The other way that XPConnect sort of relies on those conditions is that we change the JSClass when morphing from a slimwrapper to a XCWrappedNative, with the existing conditions the JSClass change didn't mean a new scope was needed. If I understand this change correctly we need to either find a way to stop changing the JSClass or we need a way to signal the change to the JS engine so it can give the object a new scope?
http://hg.mozilla.org/mozilla-central/rev/af649bce14da
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Did we want to merge this before we figured out how to avoid the regression on dealing with slimwrapper promotion?
Depends on: 517196
I think we want to back this out at least from trunk given the bugs linked to in the dependency list.
http://hg.mozilla.org/mozilla-central/rev/bc337dee7e68 backs out this on mozilla-central per comment 9 and comment 10.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

9 years ago
Priority: -- → P1
What's next here?
This was backed out from TM too:

http://hg.mozilla.org/tracemonkey/rev/bc337dee7e68
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 14

9 years ago
Stealing.
Assignee: gal → jorendorff

Updated

9 years ago
Priority: P1 → P2
(Assignee)

Comment 15

9 years ago
Created attachment 409197 [details] [diff] [review]
v1

New try, keeping a JSClass* in each emptyScope.
Attachment #396574 - Attachment is obsolete: true
Attachment #409197 - Flags: review?(gal)

Updated

9 years ago
Priority: P2 → P1
(Assignee)

Updated

9 years ago
Blocks: 514454
(Assignee)

Comment 16

9 years ago
Review ping.
Comment on attachment 409197 [details] [diff] [review]
v1

>@@ -290,18 +290,17 @@ js_FillPropertyCache(JSContext *cx, JSOb
>                      * matching empty scope. In unusual cases involving
>                      * __proto__ assignment we may not find one.
>                      */
>                     JSObject *proto = STOBJ_GET_PROTO(obj);
>                     if (!proto || !OBJ_IS_NATIVE(proto))
>                         return JS_NO_PROP_CACHE_FILL;
>                     JSScope *protoscope = OBJ_SCOPE(proto);
>                     if (!protoscope->emptyScope ||
>-                        !js_ObjectIsSimilarToProto(cx, obj, obj->map->ops, OBJ_GET_CLASS(cx, obj),
>-                                                   proto)) {
>+                        protoscope->emptyScope->clasp != obj->getClass()) {
>                         return JS_NO_PROP_CACHE_FILL;

>+    /*
>+     * Make sure proto's scope's emptyScope is available to be shared by
>+     * objects of this class.  JSScope::emptyScope is a one-slot cache. If we
>+     * omit this, some other class could snap it up. (The risk is particularly
>+     * great for Object.prototype.)
>+     *
>+     * All callers of JSObject::initSharingEmptyScope depend on this.
>+     */
>+    JSScope *scope;
>+    scope = OBJ_SCOPE(proto)->getEmptyScope(cx, clasp);

Initialize scope and you get a warning, but perhaps it's time to change the control flow to use if-then so prefetching gets the hot path. Then we lose ugly gotos and win initialization.

>   public:
>     explicit JSScope(const JSObjectOps *ops, JSObject *obj = NULL)
>       : JSObjectMap(ops, 0), object(obj) {}

[snip]

>+struct JSEmptyScope : public JSScope {

Nit: we've started putting the { on its own line for subclass/substruct decls.

>+    JSClass * const clasp;
>+
>+    explicit JSEmptyScope(const JSObjectOps *ops, JSClass *clasp)
>+        : JSScope(ops), clasp(clasp) {}

Nit: half-indent the last line.

>+inline JSEmptyScope *
>+JSScope::getEmptyScope(JSContext *cx, JSClass *clasp)
>+{
>+    if (emptyScope) {
>+        JS_ASSERT(clasp == emptyScope->clasp);
>+        emptyScope->hold();
>+        return emptyScope;
>+    }
>+    return createEmptyScope(cx, clasp);
>+}

The patch removes a call to getEmptyScopeShape from JSScope::clear, instead generating a fresh shape. Is that optimal for window objects (which are one of the few objects that are JS_ClearScope'ed)? The immediate prototype object of a window object may not be a class proto.

This seems like a good approach but it shifts some costs around. Any noticeable perf effects?

/be
(Assignee)

Updated

9 years ago
Attachment #409197 - Flags: review?(gal) → review?(graydon)
(Assignee)

Comment 18

9 years ago
Comment on attachment 409197 [details] [diff] [review]
v1

Brendan's comments are good and I'll address them. No time right now.

Shifting review to Graydon.
(Assignee)

Comment 19

9 years ago
Graydon pointed out that in js_String_tn -> js_NewObjectWithClassProto -> initSharingEmptyScope, it is not obvious that the proto there is in fact the result of js_InitClass.

It turns out that assumption *can* be wrong, if the global object's class does not have JSCLASS_GLOBAL_FLAGS; or if the user has been foolishly playing with JS_SetReservedSlot and the global object.

What should I do about that?
Comment on attachment 409197 [details] [diff] [review]
v1

I bothered jorendorff on IRC and he explained this patch to me laboriously over an hour or so, and it seems to make sense. I can't vouch for it catching all cases or even necessarily maintaining important SM invariants (few of which I have any clue about), but it at least makes as much sense to me as anything else in this code. Prototype scopes can only have their empty scope reused if the reuser is of the same class as that empty scope. What could go wrong?
Attachment #409197 - Flags: review?(graydon) → review+
(Assignee)

Comment 21

9 years ago
(In reply to comment #19)

Answer: Check at record time that the proto object we're baking into the trace already has the right OBJ_SCOPE(proto)->emptyScope->clasp.

Will push this tomorrow.
(Assignee)

Comment 22

9 years ago
Created attachment 413125 [details] [diff] [review]
v2
Attachment #409197 - Attachment is obsolete: true
Attachment #413125 - Flags: review?(brendan)
Comment on attachment 413125 [details] [diff] [review]
v2

>+    closure->initSharingEmptyScope(&js_FunctionClass, proto, parent,
>+                                   reinterpret_cast<jsval>(fun));

Shouldn't that last arg by OBJECT_TO_JSVAL(FUN_OBJECT(fun)) -- or OBJECT_TO_JSVAL(fun) if the new inline C++ helper means the upcast can be implicit?

>+    /*
>+     * Make sure proto's scope's emptyScope is available to be shared by
>+     * objects of this class.  JSScope::emptyScope is a one-slot cache. If we
>+     * omit this, some other class could snap it up. (The risk is particularly
>+     * great for Object.prototype.)
>+     *
>+     * All callers of JSObject::initSharingEmptyScope depend on this.
>+     */
>+    if (JSScope *scope = OBJ_SCOPE(proto)->getEmptyScope(cx, clasp))
>+        scope->drop(cx, NULL);
>+    else
>+        goto bad;

The else-goto is unusual style, it oddly splits the normal control flow between the consequent and the if/else's continuation. Lose the declaration in condition and invert? Or make an inline helper to hide the drop, even better.

r=me with these addressed -- happy to look at a final patch or just read the hgweb (bugzilla interdiff failed between v1 and v2 but if it works that's easier than hgweb).

/be
Attachment #413125 - Flags: review?(brendan) → review+
(Assignee)

Comment 24

9 years ago
Created attachment 413136 [details] [diff] [review]
approximate interdiff from v1 to v2
Comment on attachment 413125 [details] [diff] [review]
v2

>+    // Since the compiled code may end up passing the proto to
>+    // JSObject::initSharingEmptyScope, we can only proceed if proto has a

"proceed only"

>+    // matching emptyScope.
>+    if (key != JSProto_Array) {

Mention why JSProto_Array is excluded from the logic being commented upon here?

>+        if (!OBJ_IS_NATIVE(proto))
>+            RETURN_STOP("non-native class prototype");
>+        JSEmptyScope *emptyScope = OBJ_SCOPE(proto)->emptyScope;
>+        if (!emptyScope || JSCLASS_CACHED_PROTO_KEY(emptyScope->clasp) != key)
>+            RETURN_STOP("class prototype is not the standard one");
>+    }
>+
>     proto_ins = INS_CONSTOBJ(proto);
>     return RECORD_CONTINUE;
> }

Thanks for the approx. interdiff, it helped.

/be
(Assignee)

Comment 26

9 years ago
(In reply to comment #23)
> (From update of attachment 413125 [details] [diff] [review])
> >+    closure->initSharingEmptyScope(&js_FunctionClass, proto, parent,
> >+                                   reinterpret_cast<jsval>(fun));
> 
> Shouldn't that last arg by OBJECT_TO_JSVAL(FUN_OBJECT(fun)) -- or
> OBJECT_TO_JSVAL(fun) if the new inline C++ helper means the upcast can be
> implicit?

It's the value to store in the private slot of `closure`. It's not really a jsval; we should change all those to void * someday.
(Assignee)

Comment 27

9 years ago
Created attachment 413155 [details] [diff] [review]
v3
Attachment #413125 - Attachment is obsolete: true
Attachment #413136 - Attachment is obsolete: true
Comment on attachment 413155 [details] [diff] [review]
v3

Looks good, thanks -- brute force narrow scope via explicit block works for me.

/be
Attachment #413155 - Flags: review+
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-tracemonkey

Comment 30

9 years ago
http://hg.mozilla.org/mozilla-central/rev/851d45c347ba
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Duplicate of this bug: 514454

Comment 33

9 years ago
I had to back this out of the branch because it caused plugin crashes on the mac tinderboxen only. Will attach branch patch and investigate.
status1.9.2: final-fixed → ---

Comment 34

9 years ago
Created attachment 418849 [details] [diff] [review]
branch patch

Comment 35

9 years ago
JS_Assert + 67 (jsutil.cpp:69)
OBJ_SCOPE + 95 (jsscope.h:347)
js_InitClass + 1169 (jsobj.cpp:2973)
JS_InitClass + 157 (jsapi.cpp:2734)
jsj_init_JavaObject + 97 (jsj_JavaObject.c:1055)
JSJ_InitJSContext + 24 (jsj.c:538)
nsJVMManager::InitLiveConnectClasses(JSContext*, JSObject*) + 32 (nsJVMManager.cpp:478)

Comment 36

9 years ago
Created attachment 418867 [details] [diff] [review]
add a check js_InitClass

Waldo theorizes:

<Waldo>	so, I wonder if that patch just happened to break all classes with non-native protos
<Waldo>	and we never noticed because we don't have any
<Waldo> sayrer: what happens if you prefix the new block in js_InitClass with |if (OBJ_IS_NATIVE(proto))| ?

If I add that check, as in this patch, plugins do work.
Attachment #418867 - Flags: review?(igor)

Updated

9 years ago
Attachment #418867 - Flags: review?(mrbkap)

Updated

9 years ago
Attachment #418867 - Flags: superreview?(brendan)
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [branch patch needs review]

Updated

9 years ago
Attachment #418867 - Flags: review?(mrbkap) → review+

Updated

9 years ago
Attachment #418867 - Flags: review?(igor) → review+

Updated

9 years ago
Attachment #418867 - Flags: superreview?(brendan)
You need to log in before you can comment on or make changes to this bug.