Last Comment Bug 535416 - Eliminate JSClass::reserveSlots (was: shape guarantees do not cover scope->freeslot)
: Eliminate JSClass::reserveSlots (was: shape guarantees do not cover scope->fr...
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: P1 normal (vote)
: mozilla2.0b1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on: 574280 576730
Blocks: 558451 573651 573963
  Show dependency treegraph
 
Reported: 2009-12-16 15:49 PST by Jason Orendorff [:jorendorff]
Modified: 2010-08-20 09:10 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (26.59 KB, patch)
2010-06-16 00:04 PDT, Brendan Eich [:brendan]
gal: review+
Details | Diff | Review
proposed fix, v2 (33.80 KB, patch)
2010-06-18 14:52 PDT, Brendan Eich [:brendan]
gal: review+
Details | Diff | Review
diff of patches (9.73 KB, patch)
2010-06-18 15:13 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
preview of proposed fix, v3 (76.32 KB, patch)
2010-06-21 19:09 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
proposed fix, v3 (76.90 KB, patch)
2010-06-22 02:17 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
proposed fix, v4 (88.54 KB, patch)
2010-06-22 19:14 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
proposed fix, v4 (88.54 KB, patch)
2010-06-23 08:10 PDT, Brendan Eich [:brendan]
gal: review+
Details | Diff | Review
proposed fix, v5 (88.58 KB, patch)
2010-06-23 12:38 PDT, Brendan Eich [:brendan]
gal: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2009-12-16 15:49:00 PST
In js_GetMutableScope, if obj has a JSClass::reserveSlots hook, we can end up changing OBJ_SCOPE(obj)->freeslot without changing OBJ_SHAPE(obj).

This requires us to check, both in JSOP_SETPROP and in js_AddProperty, that the next free slot in the object matches sprop->slot for the sprop we want to add. If they don't match, we fall back to slower code. But in both cases we have already guarded on the shape of the object in question. If we strengthen the shape guarantees to cover freeslot, we can eliminate the slot checks.
Comment 1 Brendan Eich [:brendan] 2010-06-14 19:10:29 PDT
Talked to jorendorff, this really is all about eliminating JSClass::reserveSlots, which aside from any embedders silly enough to use it, is used by Function, Call, Block, and Arguments classes.

Resummarizing, more in a bit.

/be
Comment 2 Brendan Eich [:brendan] 2010-06-15 16:49:03 PDT
Part of eliminating reserveSlots is easy: the Function (flat closure only, actually), Call, Block, and Arguments objects have specialized constructors that (can in one case, already do in the others) js_EnsureReservedSlots to the exact number of slots needed, allocating dslots.

But this does not get a mutable scope, and it must not or we'll take a perf bath allocating scopes when no need for them exists ever in the life of the object. I.e., no expando properties are added for most Call objects. But one could be added: eval("var x=42") in a function makes an expando on the Call object (eval of course makes the function heavyweight at compile time, so that it gets a Call object whenever it is activated), whose slot must come after the reserved slots.

So even without reserveSlots, the bug exists. So I should restore the original summary, but there's a reason not to:

The fix as discussed yesterday but not recorded here was for the compiler to create property tree paths for all args and vars for function objects, usable by their local name mappings as well as by Call objects created for their activations. We do this already for Block object properties. For Arguments objects we could create index-named properties for [0, fun->nargs) at compile time.

The compiler creating property tree paths means the specialized constructor can set the pre-existing property tree path as the newborn's props member -- but only after bug 558451 is more done. Otherwise we need to allocate a JSScope to hold the lastProp pointer; said scope is pointed at by the newborn's JSObject::map field. Which means we take a perf bath again.

So I will leave this bug as resummarized and patch to remove reserveSlots only. The rest of the "shape guarantees do not cover scope->freeslot" must happen in bug 558451, which eliminates JSScope::freeslot as a data member and fuses scopes and sprops.

/be
Comment 3 Brendan Eich [:brendan] 2010-06-16 00:04:21 PDT
Created attachment 451516 [details] [diff] [review]
proposed fix
Comment 4 Andreas Gal :gal 2010-06-16 00:20:41 PDT
Comment on attachment 451516 [details] [diff] [review]
proposed fix

Looks good. The LOCK/UNLOCK around the memcpy is probably unnecessary. The freeslot handling (#2) is wonky, but once scopes are merged into objects, we can properly give these objects (precalculated) shapes and property tree paths.
Comment 5 Brendan Eich [:brendan] 2010-06-18 14:52:40 PDT
Created attachment 452357 [details] [diff] [review]
proposed fix, v2
Comment 6 Brendan Eich [:brendan] 2010-06-18 15:13:51 PDT
Created attachment 452363 [details] [diff] [review]
diff of patches

Interdiff fails.

/be
Comment 7 Andreas Gal :gal 2010-06-18 15:50:14 PDT
Comment on attachment 452357 [details] [diff] [review]
proposed fix, v2

The open-coding is really ugly. I hope we get rid of that eventually. Also, I don't like the dslot allocation in the NewObject path, but you mentioned we can get rid of that soon, too.
Comment 8 Brendan Eich [:brendan] 2010-06-18 16:09:19 PDT
Open-coded emptyFooScope stuff going away, waitforit!

/be
Comment 9 Brendan Eich [:brendan] 2010-06-21 19:09:49 PDT
Created attachment 452945 [details] [diff] [review]
preview of proposed fix, v3

Still testing, this also splits cases to avoid unpredictable branches in the object allocation paths.

/be
Comment 10 Brendan Eich [:brendan] 2010-06-22 02:17:27 PDT
Created attachment 453024 [details] [diff] [review]
proposed fix, v3
Comment 11 Brendan Eich [:brendan] 2010-06-22 19:14:53 PDT
Created attachment 453274 [details] [diff] [review]
proposed fix, v4
Comment 12 Brendan Eich [:brendan] 2010-06-23 08:10:53 PDT
Created attachment 453381 [details] [diff] [review]
proposed fix, v4

Passes tryserver. Want to land ASAP.

Note comments around NewObject calls in jsfun.cpp and jsobj.cpp (js_InitClass) -- followup work required to get rid of unnecessary/unpredictable branches in NewObject{,WithGivenProto}.

This patch does as much as fits to improve the branch situation by introducing NewBuiltinClassInstance and NewNativeClassInstance. Comments in jsobjinlines.h (another followup: less C-ish HelperFunction style, more C++).


/be
Comment 13 Andreas Gal :gal 2010-06-23 10:23:06 PDT
Comment on attachment 453381 [details] [diff] [review]
proposed fix, v4

+static inline JSObject *
>+NewDenseArrayObject(JSContext *cx)
>+{
>+    return NewObject(cx, &js_ArrayClass, NULL, NULL);
>+}
>+

Why the NULLs here? Shouldn't this be some more specialized case? Dense arrays are very frequent.

>+    /*
>+     * We must wrap funobj with a JSFunction, so use NewObjectWithGivenProto.
>+     * This helper has a special case for js_FunctionClass, triggered here and
>+     * also possibly via the JS_NewObjectForGivenProto and JS_NewObject APIs.
>+     */
>     JSObject *wfunobj = NewObjectWithGivenProto(cx, &js_FunctionClass,
>                                                 funobj, scopeChain);

That special case is very ugly btw. I wanted to change that.

>     JS_ASSERT(parent);
>     JS_ASSERT(proto);

We should also assert this in NewNativeClassInstance. Only the new NewNonEscapingObject thing should permit null proto/parent.

>-        obj = NewObjectWithGivenProto(cx, &js_NoSuchMethodClass, NULL, NULL);
>+        obj = js_NewGCObject(cx);
>         if (!obj)
>             return false;
>+        obj->map = NULL;
>+        obj->classword = jsuword(&js_NoSuchMethodClass);
>+        obj->setProto(NULL);
>+        obj->setParent(NULL);
>         obj->fslots[JSSLOT_FOUND_FUNCTION] = tvr.value();
>         obj->fslots[JSSLOT_SAVED_ID] = vp[0];
>         vp[0] = OBJECT_TO_JSVAL(obj);
>     }
>     return true;

NewNonEscapingObject ftw!

>-static JSObject *
>-NewIteratorObject(JSContext *cx, uintN flags)
>+static inline JSObject *
>+NewIteratorObject(JSContext *cx)
> {
>-    return !(flags & JSITER_ENUMERATE)
>-           ? NewObject(cx, &js_IteratorClass.base, NULL, NULL)
>-           : NewObjectWithGivenProto(cx, &js_IteratorClass.base, NULL, NULL);
>+    return NewBuiltinClassInstance(cx, &js_IteratorClass.base);
> }

This is de-optimizing the code. We can use explicit NULL/NULL proto/parent if its not escaping, but we need the real proto if it can escape. Brendan says this avoids making a new scope every time. However, once we have eliminated scopes we should maybe go back to this?

>+bool
>+js::FindClassPrototype(JSContext *cx, JSObject *scope, JSProtoKey protoKey, JSObject **protop,
>+                       JSClass *clasp)
>+{
>+    jsval v;
>+    if (!js_FindClassObject(cx, scope, protoKey, &v, clasp))
>+        return false;
>+
>+    if (VALUE_IS_FUNCTION(cx, v)) {
>+        JSObject *ctor = JSVAL_TO_OBJECT(v);
>+        if (!ctor->getProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.classPrototypeAtom), &v))
>+            return false;
>+        if (!JSVAL_IS_PRIMITIVE(v)) {
>+            /*
>+             * Set the newborn root in case v is otherwise unreferenced. It is
>+             * ok to overwrite newborn roots here, since the getter called just
>+             * above could have.
>+             *
>+             * Unlike the common GC rooting model, our callers do not have to
>+             * protect protop thanks to this set newborn root, since they all
>+             * immediately create a new instance that delegates to this object,
>+             * or just query the prototype for its class.
>+             */
>+            cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = JSVAL_TO_OBJECT(v);

We officially started relying on the conservative scanner now, so this can go. Or separate patch. Up to you.

>+/*
>+ * Create an instance of any class, native or not, JSFunction-sized or not.
>+ *
>+ * If proto is null, use the memoized original value of the class constructor
>+ * .prototype property object for a built-in class, else the current value of
>+ * .prototype if available, else Object.prototype.
>+ *
>+ * Default parent is null to proto's parent (null if proto is null too).
>+ */
> static inline JSObject *
>-NewObject(JSContext *cx, JSClass *clasp, JSObject *proto,
>-          JSObject *parent, size_t objectSize = 0)
>+NewObject(JSContext *cx, JSClass *clasp, JSObject *proto, JSObject *parent)

How many uses do we have left? Maybe we should delete this eventually.
Comment 14 Brendan Eich [:brendan] 2010-06-23 12:35:05 PDT
(In reply to comment #13)
> (From update of attachment 453381 [details] [diff] [review])
> +static inline JSObject *
> >+NewDenseArrayObject(JSContext *cx)
> >+{
> >+    return NewObject(cx, &js_ArrayClass, NULL, NULL);
> >+}
> >+
> 
> Why the NULLs here? Shouldn't this be some more specialized case? Dense arrays
> are very frequent.

I wish -- see comment 12, and the jsobjinlines.h commentary about how NewBuiltinClassInstance cannot be used if the class prototype has a different JSClass from the instance. Array.prototype is slowified. This is followup fodder (I will file today).

> >+    /*
> >+     * We must wrap funobj with a JSFunction, so use NewObjectWithGivenProto.
> >+     * This helper has a special case for js_FunctionClass, triggered here and
> >+     * also possibly via the JS_NewObjectForGivenProto and JS_NewObject APIs.
> >+     */
> >     JSObject *wfunobj = NewObjectWithGivenProto(cx, &js_FunctionClass,
> >                                                 funobj, scopeChain);
> 
> That special case is very ugly btw. I wanted to change that.

You and me both. I took a swing at it last night but it's out of scope here, and a bunch of work.

> >     JS_ASSERT(parent);
> >     JS_ASSERT(proto);
> 
> We should also assert this in NewNativeClassInstance.

These asserts are in NewNativeClassInstance, which is called by NewBuiltinClassInstance so that's covered too (it computes the correct original proto value, and uses proto->getParent(), as noted).

> Only the new
> NewNonEscapingObject thing should permit null proto/parent.

See next patch. The non-escaping cases do not yet generalize this way, so I used null map only for the NoSuchMethod internal helper object.

> This is de-optimizing the code.

Probably not -- finding the default proto for Iterator in the global object is faster than creating a scope for each instance. But I fixed, see next patch.

> >+bool
> >+js::FindClassPrototype(JSContext *cx, JSObject *scope, JSProtoKey protoKey, JSObject **protop,
> >+                       JSClass *clasp)
> >+{
> >+    jsval v;
> >+    if (!js_FindClassObject(cx, scope, protoKey, &v, clasp))
> >+        return false;
> >+
> >+    if (VALUE_IS_FUNCTION(cx, v)) {
> >+        JSObject *ctor = JSVAL_TO_OBJECT(v);
> >+        if (!ctor->getProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.classPrototypeAtom), &v))
> >+            return false;
> >+        if (!JSVAL_IS_PRIMITIVE(v)) {
> >+            /*
> >+             * Set the newborn root in case v is otherwise unreferenced. It is
> >+             * ok to overwrite newborn roots here, since the getter called just
> >+             * above could have.
> >+             *
> >+             * Unlike the common GC rooting model, our callers do not have to
> >+             * protect protop thanks to this set newborn root, since they all
> >+             * immediately create a new instance that delegates to this object,
> >+             * or just query the prototype for its class.
> >+             */
> >+            cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = JSVAL_TO_OBJECT(v);
> 
> We officially started relying on the conservative scanner now, so this can go.
> Or separate patch. Up to you.

I just moved this code, but I would like to get rid of this noise. Ok, doing in the last minute.

> >+/*
> >+ * Create an instance of any class, native or not, JSFunction-sized or not.
> >+ *
> >+ * If proto is null, use the memoized original value of the class constructor
> >+ * .prototype property object for a built-in class, else the current value of
> >+ * .prototype if available, else Object.prototype.
> >+ *
> >+ * Default parent is null to proto's parent (null if proto is null too).
> >+ */
> > static inline JSObject *
> >-NewObject(JSContext *cx, JSClass *clasp, JSObject *proto,
> >-          JSObject *parent, size_t objectSize = 0)
> >+NewObject(JSContext *cx, JSClass *clasp, JSObject *proto, JSObject *parent)
> 
> How many uses do we have left? Maybe we should delete this eventually.

We have a bunch of uses still (grep -w with my patch shows 16), and also the API uses this directly. It's not going away easily but I am taking a big step to reduce internal usage of NewObject (at least 30 were converted to NewBuiltinClassInstance or NewNativeClassInstance, plus a couple of direct js_NewGCObject fast paths now).

/be
Comment 15 Brendan Eich [:brendan] 2010-06-23 12:38:45 PDT
Created attachment 453469 [details] [diff] [review]
proposed fix, v5
Comment 16 Gary Kwong [:gkw] [:nth10sd] 2010-06-24 00:07:18 PDT
This was checked in to TM:

http://hg.mozilla.org/tracemonkey/rev/8c2faceba7bf

Note You need to log in before you can comment on or make changes to this bug.