Closed Bug 492355 Opened 11 years ago Closed 11 years ago

Suspected Txul regression from JS engine changes

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: bzbarsky, Assigned: brendan)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 5 obsolete files)

One of bug 490818, bug 490191, bug 452189 seems to have caused a 4.5% Txul regression on Mac.  Or maybe it was all three together.  In any case, this needs to be looked into.
Flags: blocking1.9.2?
Flags: blocking1.9.1?
I added comments to each of the patches in question to make sure that they don't land on 191 without the issue being addressed.
I bet it is the patch for bug 452189 -- I'll investigate and report back.

/be
Status: NEW → ASSIGNED
Fix coming up.

/be
No longer blocks: 490191, 490818
Flags: wanted1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
Hmm, harder than I thought. Working on it, will have a patch today some time.

/be
This helps, but XPConnect has a nasty habit of lazily binding setters on wrapped native prototypes after those protos have been implicated in property cache fill operations. Really want a way of avoiding purging if there is no name shadowing going on. It may be worth keeping track of names somehow.

/be
Igor, your comments welcome as usual.

/be
(In reply to comment #5)
> Created an attachment (id=376870) [details]

Perhaps checking the shape of the end of the prototype chain for predictive property adds could be an option?
(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=376870) [details] [details]
> 
> Perhaps checking the shape of the end of the prototype chain for predictive
> property adds could be an option?

Thought about that -- would fix the single-proto case shown in this bug. Would not help deeper situations where there was no shadowing of a property in the proto at the end of the chain.

For not-found property tracing we guard all shapes along the prototype chain. Not something the cache can do.

Good point about add (setMissing) rather than set (setExisting) being the operation here.

/be
(In reply to comment #8)
> 
> For not-found property tracing we guard all shapes along the prototype chain.
> Not something the cache can do.

The cache for setMissing just needs to store the shape of the final end of the prototype chain since mutations of any intermediate nodes would change that as well. That shape, I presume, can be stored in entry->vcap - it is not used for setMissing hits AFAICS.
Intermediate adds do not reshape subsequent objects on the chain if those objects do not have a shadowed property (same id).

/be
(In reply to comment #10)
> Intermediate adds do not reshape subsequent objects on the chain if those
> objects do not have a shadowed property (same id).

Perhaps such adds to the delagates could mutate the shape for the end of the property chain (and only it)?

Alternatively, such adds-to-delagates can mutate single per-runtime shape that the cache can compare. This would also be fastest to check in the property cache.
(In reply to comment #11)
> Alternatively, such adds-to-delagates can mutate single per-runtime shape that
> the cache can compare. This would also be fastest to check in the property
> cache.

Note that check would only be necessary for the setMissing case.
(In reply to comment #12)
> (In reply to comment #11)
> > Alternatively, such adds-to-delagates can mutate single per-runtime shape that
> > the cache can compare. This would also be fastest to check in the property
> > cache.
> 
> Note that check would only be necessary for the setMissing case.

Also the mutation of this global shape would only be necessary when adding setters or read-only properties.
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Alternatively, such adds-to-delagates can mutate single per-runtime shape that
> > > the cache can compare. This would also be fastest to check in the property
> > > cache.
> > 
> > Note that check would only be necessary for the setMissing case.
> 
> Also the mutation of this global shape would only be necessary when adding
> setters or read-only properties.

It would not be the global's shape, it would be Object.prototype's shape. The issue is prototype-based delegation.

For the scope chain, since you fixed the problem of native objects at the front that can be accessed by exposed references (not only by the scope chain's parent link), there is no such problem.

/be
(In reply to comment #14)
> It would not be the global's shape, it would be Object.prototype's shape. The
> issue is prototype-based delegation.

By "glibal shape" I meant to have a separated JSRuntime.shapeOfSomething that would be mutated whenever a setter or a read-only property is added to a delegate object. Then FillPropertyCache would store this in entry->vcap when caching added properties. The setMissed case of JSOP_SETPROP would check that the cached shapeOfSomething matches the current value.
Sorry, AFK -- yeah, I was thinking the exact same thing: rt->protoHazardShape or some such. Will hack it up.

/be
Attached patch proposed fix (non-minimal) (obsolete) — Splinter Review
I kept cleanup from the first attachment, including removing a left-over comment that moved elsewhere in jscntxt.h, and improving some other comments and packing some JSPackedBool members. I can split these out if it's considered necessary but as usual want to make haste without waste, where possible.

Still have some testing to do on Txul, but I'll rely on tryservers. Rob, could you help me by queueing this? I have to run an errand ASAP.
 
/be
Attachment #376870 - Attachment is obsolete: true
Attachment #377044 - Flags: review?(igor)
Comment on attachment 377044 [details] [diff] [review]
proposed fix (non-minimal)

From a quick glance:

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>+    JSObject *start = obj;
>+    if (scopeIndex != 0) {
>+        for (uintN i = 0; i != scopeIndex; i++)
>+            start = OBJ_GET_PARENT(cx, start);
>+    }
>+
>     if (protoIndex != 0) {
>-        JSObject *tmp;
>-
>-        JS_ASSERT(pobj != obj);
>+        JS_ASSERT(pobj != start);
>         protoIndex = 1;
>-        tmp = obj;
>+
>+        JSObject *tmp = start;

The start local is only used when protoIndex is not 0. This suggests to move its calculation into the if (protoIndex != 0).


>         for (;;) {
>             tmp = OBJ_GET_PROTO(cx, tmp);
> 
>@@ -180,6 +186,7 @@ js_FillPropertyCache(JSContext *cx, JSOb
>             ++protoIndex;
>         }
>     }
>+
>     if (scopeIndex > PCVCAP_SCOPEMASK || protoIndex > PCVCAP_PROTOMASK) {
>         PCMETER(cache->longchains++);
>         return JS_NO_PROP_CACHE_FILL;
>@@ -301,10 +308,13 @@ js_FillPropertyCache(JSContext *cx, JSOb
>     if (kshape == 0)
>         kshape = OBJ_SHAPE(obj);
>     khash = PROPERTY_CACHE_HASH_PC(pc, kshape);
>+    vshape = scope->shape;
>     if (obj == pobj) {
>         JS_ASSERT(scopeIndex == 0 && protoIndex == 0);
>         JS_ASSERT(OBJ_SCOPE(obj)->object == obj);
>         JS_ASSERT(kshape != 0);
>+        if (addedSprop)
>+            vshape = cx->runtime->protoHazardShape;
>     } else {
>         if (op == JSOP_LENGTH) {
>             atom = cx->runtime->atomState.lengthAtom;
>@@ -313,33 +323,36 @@ js_FillPropertyCache(JSContext *cx, JSOb
>             GET_ATOM_FROM_BYTECODE(cx->fp->script, pc, pcoff, atom);
>         }
>         JS_ASSERT_IF(scopeIndex == 0,
>-                     protoIndex != 1 || OBJ_GET_PROTO(cx, obj) == pobj);
>+                     protoIndex != 0 &&
>+                     ((protoIndex == 1)
>+                      ? OBJ_GET_PROTO(cx, obj) == pobj
>+                      : OBJ_GET_PROTO(cx, obj) != pobj));
>         if (scopeIndex != 0 || protoIndex != 1) {
>             khash = PROPERTY_CACHE_HASH_ATOM(atom, obj, pobj);
>             PCMETER(if (PCVCAP_TAG(cache->table[khash].vcap) <= 1)
>                         cache->pcrecycles++);
>             pc = (jsbytecode *) atom;
>             kshape = (jsuword) obj;
>-
>-            /*
>-             * Make sure that a later shadowing assignment will enter
>-             * PurgeProtoChain and invalidate this entry, bug 479198.
>-             *
>-             * This is thread-safe even though obj is not locked. Only the
>-             * DELEGATE bit of obj->classword can change at runtime, given that
>-             * obj is native; and the bit is only set, never cleared. And on
>-             * platforms where another CPU can fail to see this write, it's OK
>-             * because the property cache and JIT cache are thread-local.
>-             */
>-            OBJ_SET_DELEGATE(cx, obj);
>-        }
>+        }
>+
>+        /*
>+         * Ensure that a later shadowing assignment will call PurgeProtoChain
>+         * and invalidate this entry (see bug 479198).
>+         *
>+         * This is thread-safe even though obj is not locked because only the
>+         * DELEGATE (1) bit of obj->classword can change at runtime, given that
>+         * obj is a native object; and the bit is only set, never cleared; and
>+         * on platforms where another CPU can fail to see this write, it's OK
>+         * because the property cache and JIT cache are thread-local.
>+         */
>+        OBJ_SET_DELEGATE(cx, obj);
>     }
> 
>     entry = &cache->table[khash];
>     PCMETER(PCVAL_IS_NULL(entry->vword) || cache->recycles++);
>     entry->kpc = pc;
>     entry->kshape = kshape;
>-    entry->vcap = PCVCAP_MAKE(scope->shape, scopeIndex, protoIndex);
>+    entry->vcap = PCVCAP_MAKE(vshape, scopeIndex, protoIndex);
>     entry->vword = vword;
> 
>     cache->empty = JS_FALSE;
>@@ -4590,7 +4603,9 @@ js_Interpret(JSContext *cx)
>                     PCMETER(cache->pctestentry = entry);
>                     PCMETER(cache->tests++);
>                     PCMETER(cache->settests++);
>-                    if (entry->kpc == regs.pc && entry->kshape == kshape) {
>+                    if (entry->kpc == regs.pc &&
>+                        entry->kshape == kshape &&
>+                        entry->vcap == PCVCAP_MAKE(rt->protoHazardShape, 0, 0)) {
>                         JSScope *scope;
> 
>                         JS_LOCK_OBJ(cx, obj);
>@@ -6303,7 +6318,9 @@ js_Interpret(JSContext *cx)
>                 PCMETER(cache->tests++);
>                 PCMETER(cache->initests++);
> 
>-                if (entry->kpc == regs.pc && entry->kshape == kshape) {
>+                if (entry->kpc == regs.pc &&
>+                    entry->kshape == kshape &&
>+                    entry->vcap == PCVCAP_MAKE(rt->protoHazardShape, 0, 0)) {
>                     PCMETER(cache->pchits++);
>                     PCMETER(cache->inipchits++);
> 
>@@ -6341,7 +6358,6 @@ js_Interpret(JSContext *cx)
>                      * obj, not a proto-property, and there cannot have been
>                      * any deletions of prior properties.
>                      */
>-                    JS_ASSERT(PCVCAP_MAKE(sprop->shape, 0, 0) == entry->vcap);
>                     JS_ASSERT(!SCOPE_HAD_MIDDLE_DELETE(scope));
>                     JS_ASSERT(!scope->table ||
>                               !SCOPE_HAS_PROPERTY(scope, sprop));
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -3746,17 +3746,21 @@ js_DefineNativeProperty(JSContext *cx, J
> #endif /* JS_HAS_GETTER_SETTER */
> 
>     /*
>-     * Purge the property cache of now-shadowed id in obj's scope chain.
>-     * Do this early, before locking obj to avoid nesting locks.
>-     *
>-     * But first, purge the entire cache if obj is a prototype (we approximate
>-     * this via OBJ_IS_DELEGATE) and we are defining a non-shadowable property
>-     * on it (see bug 452189).
>+     * Purge the property cache of any properties named by id that are about to
>+     * be shadowed in obj's scope chain. We do this before locking obj to avoid
>+     * nesting locks.
>+     */
>+    js_PurgeScopeChain(cx, obj, id);
>+
>+    /*
>+     * If a readonly property or setter is being defined on a prototype object
>+     * implicated in property cache fills and the trace-JITted code induced by
>+     * certain cache fills, increment rt->protoHazardShape, which stands in for
>+     * cache entries' vcap members, and in JITted shape guards, for the shapes
>+     * of all such prototypes.
>      */
>     if (OBJ_IS_DELEGATE(cx, obj) && (attrs & (JSPROP_READONLY | JSPROP_SETTER)))
>-        js_PurgePropertyCache(cx, &JS_PROPERTY_CACHE(cx));
>-    else
>-        js_PurgeScopeChain(cx, obj, id);
>+        JS_ATOMIC_INCREMENT(&cx->runtime->protoHazardShape);


This does not check for this pseudo-shape overflow which quite possible with 24-bit shapes. Using js_GenerateShape here should be more sound. But that would require to update the shape during the GC.

I will review the rest of the patch tomorrow.
Igor, thanks -- good comments, but you caught the gal overcite disease ;-). New patch before tomorrow.

/be
(In reply to comment #20)
> you caught the gal overcite disease ;-)

I haven't cited the whole patch. It implies that the virus has mutated resulting in milder illness ;)
Comment on attachment 377044 [details] [diff] [review]
proposed fix (non-minimal)

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>@@ -301,10 +308,13 @@ js_FillPropertyCache(JSContext *cx, JSOb
>+    vshape = scope->shape;
>     if (obj == pobj) {
>         JS_ASSERT(scopeIndex == 0 && protoIndex == 0);
>         JS_ASSERT(OBJ_SCOPE(obj)->object == obj);
>         JS_ASSERT(kshape != 0);
>+        if (addedSprop)
>+            vshape = cx->runtime->protoHazardShape;

Preexisting nit: my choice of addedSprop as a name of the parameter was not precise. setMissing would be much better reflection of the intended use here.

Another observation is that in this case of (obj == pobj) the property cache testing code in PROPERTY_CACHE_TEST and js_Interpret checks vshape twice. First as entry->kshape == kshape and then when double-checking that PCVCAP_SHAPE(entry->vcap) == OBJ_SHAPE(pobj). This could be optimized, but it is for another bug. 


>@@ -313,33 +323,36 @@ js_FillPropertyCache(JSContext *cx, JSOb
>         JS_ASSERT_IF(scopeIndex == 0,
>-                     protoIndex != 1 || OBJ_GET_PROTO(cx, obj) == pobj);
>+                     protoIndex != 0 &&
>+                     ((protoIndex == 1)
>+                      ? OBJ_GET_PROTO(cx, obj) == pobj
>+                      : OBJ_GET_PROTO(cx, obj) != pobj));

Nit: given the complexity of the assert "#ifdef DEBUG", the explicit if (scopeIndex == 0) and separated asserts that would add clarity.

>@@ -4590,7 +4603,9 @@ js_Interpret(JSContext *cx)
>-                    if (entry->kpc == regs.pc && entry->kshape == kshape) {
...
>+                        entry->vcap == PCVCAP_MAKE(rt->protoHazardShape, 0, 0)) {

Nit: Checking for PCVCAP_SHAPE(entry->vcap) == rt->protoHazardShape together with an assert that PCVCAP_TAG(entry->vcap) is zero would improve assert coverage.

>@@ -6303,7 +6318,9 @@ js_Interpret(JSContext *cx)
>+                if (entry->kpc == regs.pc &&
>+                    entry->kshape == kshape &&
>+                    entry->vcap == PCVCAP_MAKE(rt->protoHazardShape, 0, 0)) {

Same here.
(In reply to comment #19)
> >+        JS_ATOMIC_INCREMENT(&cx->runtime->protoHazardShape);
> 
> 
> This does not check for this pseudo-shape overflow which quite possible with
> 24-bit shapes. Using js_GenerateShape here should be more sound. But that would
> require to update the shape during the GC.

This is a good point and the fix is to js_GenerateShape what is then assigned to rt->protoHazardShape. This avoids aliasing proto-hazard shapes and regular shapes (replaying a live shape as a proto-hazard shape in the cache or a guard could lead to a misjudgment).

Then we need js_CompareAndSwap to update rt->protoHazardShape, which we have. New patch soon.

/be
(In reply to comment #22)
> Another observation is that in this case of (obj == pobj) the property cache
> testing code in PROPERTY_CACHE_TEST and js_Interpret checks vshape twice. First
> as entry->kshape == kshape and then when double-checking that
> PCVCAP_SHAPE(entry->vcap) == OBJ_SHAPE(pobj). This could be optimized, but it
> is for another bug.

Filed as bug 492838.

/be
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Attachment #377044 - Attachment is obsolete: true
Attachment #377248 - Flags: review?(igor)
Attachment #377044 - Flags: review?(igor)
Comment on attachment 377248 [details] [diff] [review]
proposed fix, v2

> static inline void
>-js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope) {
>+js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope)
>+{

Nit: a style-only change should make the style the right one and the style of the pointer type in this file is X *, not X*. 

The rest of comments are on irc.
Attachment #377248 - Flags: review?(igor) → review-
(In reply to comment #26)
> (From update of attachment 377248 [details] [diff] [review])
> > static inline void
> >-js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope) {
> >+js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope)
> >+{
> 
> Nit: a style-only change should make the style the right one and the style of
> the pointer type in this file is X *, not X*. 

I will let this lie since nearby functions use the same style (they may not have originally, but they do now).

/be
Attached patch proposed fix, v3 (obsolete) — Splinter Review
Attachment #377248 - Attachment is obsolete: true
Attachment #377268 - Flags: review?(igor)
Comment on attachment 377268 [details] [diff] [review]
proposed fix, v3

>@@ -157,12 +157,17 @@ js_FillPropertyCache(JSContext *cx, JSOb
>     if (protoIndex != 0) {
...
>+        if (scopeIndex != 0) {
>+            for (uintN i = 0; i != scopeIndex; i++)
>+                tmp = OBJ_GET_PARENT(cx, tmp);
>+        }

Nit: no need for the wrapping "if" around the loop.

>@@ -193,6 +199,7 @@ js_FillPropertyCache(JSContext *cx, JSOb
...
>     kshape = 0;
>+    vshape = scope->shape;

This reads the shape too early: the branding case below under do-while updates the shape. That is the reason the code uses kshape = 0 here.

>@@ -294,6 +301,14 @@ js_FillPropertyCache(JSContext *cx, JSOb
>                     if (proto && OBJ_IS_NATIVE(proto))
>                         kshape = OBJ_SHAPE(proto);
>                 }
>+
>+                /*
>+                 * When adding we predict no prototype object will later gain a
>+                 * readonly property or setter. If that kind of hazard appears,
>+                 * rt->protoHazardShape will be regenerated, thereby purging
>+                 * any property cache entries and causing trace guards to exit.
>+                 */

Nit: Ideally the comment here or somewhere else should explain why adding a setter or read-only property must inhibit the use of the cached sprop. But even a reference to this bug should be OK.

>@@ -312,8 +327,16 @@ js_FillPropertyCache(JSContext *cx, JSOb
>+#ifdef DEBUG
>+        if (scopeIndex == 0) {
>+            JS_ASSERT(protoIndex != 0);
>+            JS_ASSERT((protoIndex == 1)
>+                      ? OBJ_GET_PROTO(cx, obj) == pobj
>+                      : OBJ_GET_PROTO(cx, obj) != pobj);
>+        }

For my eyes the second assert looks easy to follow when written as
              JS_ASSERT((protoIndex == 1) == (OBJ_GET_PROTO(cx, obj) == pobj));

But that could be due to that fit-one-line feeling. 

>+    /*
>+     * If a readonly property or setter is being defined on a prototype object
>+     * implicated in property cache fills and the trace-JITted code induced by
>+     * certain cache fills, increment rt->protoHazardShape, which stands in for
>+     * cache entries' vcap members, and in JITted shape guards, for the shapes
>+     * of all such prototypes.
>      */

I guess this comment, the comment in js_FillProtpertyCache and the comment in JSRuntime about protoHazardShape can be done in one place. Two other places can just refer to that place.

> static inline void
>-js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope) {
>+js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope)
>+{
>     js_LeaveTraceIfGlobalObject(cx, scope->object);
>     scope->shape = js_GenerateShape(cx, JS_FALSE);
> }
> 
> static inline void
>-js_ExtendScopeShape(JSContext *cx, JSScope *scope, JSScopeProperty *sprop) {
>+js_ExtendScopeShape(JSContext *cx, JSScope *scope, JSScopeProperty *sprop)

Nite: Note that the style in the first function is T* and here it is T *. I can stand any as long as it is consistent.

>+    JS_ASSERT_IF(PCVCAP_SHAPE(entry->vcap) != scope->shape,
>+                 PCVCAP_SHAPE(entry->vcap) == cx->runtime->protoHazardShape);

The assert should be removed - another thread could have changed cx->runtime->protoHazardShape by this time.
Attachment #377268 - Flags: review?(igor) → review-
Attached patch proposed fix, v4 (obsolete) — Splinter Review
Attachment #377268 - Attachment is obsolete: true
Attachment #377279 - Flags: review?(igor)
Attachment #377279 - Flags: review?(igor) → review+
Comment on attachment 377279 [details] [diff] [review]
proposed fix, v4

> static inline void
>-js_MakeScopeShapeUnique(JSContext* cx, JSScope* scope) {
>+js_MakeScopeShapeUnique(JSContext *cx, JSScope *scope)
>+{

Nice :)

>@@ -8006,10 +8003,17 @@ TraceRecorder::record_SetPropHit(JSPropC
...
>-    if (entry->kshape != PCVCAP_SHAPE(entry->vcap)) {
>+    uint32 vshape = PCVCAP_SHAPE(entry->vcap);
>+    if (entry->kshape != vshape) {

Note for myself: this check is another reason why protoHazardShape must use js_GenerateShape and not a separated counter that could coincide in value with a scope shape.
Attached patch proposed fix, v5Splinter Review
Forgot something from the original patch for this bug: avoid making declarative environment object that is the scope parent of the new Call object a delegate prematurely (before defining the named function expression's name on the declenv object). Igor, was there a reason to split the lambdaName code up and re-test? I couldn't see a rooting reason.

/be
Attachment #377279 - Attachment is obsolete: true
Attachment #377291 - Flags: review?(igor)
(In reply to comment #32)
> was there a reason to split the lambdaName code up and re-test? I couldn't see a rooting reason.

The reason was that initially, when changing the code to make non-lazy environment initialization, I made a bad typo and mistaken callobj for fp->callee. That required to call js_DefineNativeProperty after creating the callobj. Later, when I fixed the bug, I forgot to move js_DefineNativeProperty back.
Comment on attachment 377291 [details] [diff] [review]
proposed fix, v5

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>-        /* Root env. */
>+        /* Root env before js_DefineNativeProperty (-> JSClass.addProperty). */
>         fp->scopeChain = env;
>+        if (!js_DefineNativeProperty(cx, fp->scopeChain, ATOM_TO_JSID(lambdaName),
>+                                     OBJECT_TO_JSVAL(fp->callee), NULL, NULL,
>+                                     JSPROP_PERMANENT | JSPROP_READONLY,
>+                                     0, 0, NULL)) {
>+            return NULL;
>+        }
>     }

Thanks for putting this js_DefineNativeProperty to where it belongs!
Attachment #377291 - Flags: review?(igor) → review+
Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/0827b97fb89b

/be
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/0827b97fb89b

if the regression persists, let's reopen
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Resolution: --- → FIXED
Depends on: 498565
Flags: blocking1.9.2?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.