557652 - Eliminate redundant guard that incProp/getProp operand is not the global object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

There are some cases where the tracer emits code like:

  // guard obj0 != globalObj
  17: globalObj = imml -1218629632
  18: eql2 = eql obj0, globalObj
  19: xt1: xt eql2 -> pc=0x9c2b711 imacpc=(nil) sp+8 rp+0 (GuardID=002)
  // guard obj0->map->shape == 182
  20: map = ldl.o obj0[0]
  21: shape = ldl.o map[4]
  22: 182 = imml 182
  23: guard_kshape = eql shape, 182
  24: xf2: xf guard_kshape -> pc=0x9c2b711 imacpc=(nil) sp+8 rp+0 (GuardID=003)

The first guard is unnecessary as long as the shape of globalObj is not 182.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I assume "aobj" isn't "array obj" here, because under no circumstances should we actually support an array object as a global.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

No measurable improvement in SunSpider or V8. Still, a little less code is always a good thing.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 437414 [details] [diff] [review]
v1

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -9242,16 +9242,23 @@ TraceRecorder::guardPropertyCacheHit(LIn

Note the if leading to this excerpted then clause tail is

..    if (aobj == globalObj) {
..        if (entry->adding())
..            RETURN_STOP("adding a property to the global object");

>         JSOp op = js_GetOpcode(cx, cx->fp->script, cx->fp->regs->pc);
>         if (JOF_OPMODE(op) != JOF_NAME) {
>             guard(true,
>                   addName(lir->ins2(LIR_peq, obj_ins, INS_CONSTOBJ(globalObj)), "guard_global"),
>                   exit);
>         }
>     } else {
>         CHECK_STATUS(guardShape(obj_ins, aobj, entry->kshape, "guard_kshape", exit));
>+
>+        // Avoid accessing stale slots of the global object on trace.
>+        // Ordinarily, the shape guard above confirms that obj_ins isn't the
>+        // global object. But in the unlikely case that aobj is the same shape
>+        // as globalObj, a second guard is needed.
>+        if (vshape == OBJ_SHAPE(globalObj))
>+            CHECK_STATUS(guardNotGlobalObject(aobj, obj_ins));
>     }

Just a bit further, after the entry->adding() jazz:

..    // For any hit that goes up the scope and/or proto chains, we will need to
..    // guard on the shape of the object containing the property.
..    if (entry->vcapTag() >= 1) {
..        JS_ASSERT(OBJ_SHAPE(obj2) == vshape);
..        if (obj2 == globalObj)
..            RETURN_STOP("hitting the global object via a prototype chain");

So is the new "if (vshape == OBJ_SHAPE(globalObj)) ..." code needed only for the entry->vcapTag() == 0 case?

/be

Comment 4

[Jason Orendorff [:jorendorff]]

I need to look at this patch some more. I may have moved the guard to the wrong place.

(In reply to comment #1)
> I assume "aobj" isn't "array obj" here, because under no circumstances should
> we actually support an array object as a global.

Sorry for the line noise. In a few places we use the identifier "aobj" to mean "the object where we start the lookup, or else Array.prototype, if that object happens to be a dense array".

Brendan: The case we're guarding for here isn't the case where globalObj is hit via a prototype chain -- it's an even less likely case where we're operating on an object that happens to be the same shape as the global (pretty far-fetched considering how unique the global is anyway, and how likely it is to be branded).

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Brendan: The case we're guarding for here isn't the case where globalObj is hit
> via a prototype chain -- it's an even less likely case where we're operating on
> an object that happens to be the same shape as the global (pretty far-fetched
> considering how unique the global is anyway, and how likely it is to be
> branded).

Also how global objects have their own class, distinct from other objects' classes, and so have different shapes from other objects.

We check and guard globalObj identity but I thought maybe you were concerned with a recording that found a non-global object that had the same shape as the particular globalObj at hand. Another global with no method called (not branded) could have the same shape. Two windows starting with the same content?

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2

Per discussion on IRC, insist on a unique shape for the global object before recording begins.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 437658 [details] [diff] [review]
v2

>@@ -2169,16 +2169,17 @@ TraceRecorder::TraceRecorder(JSContext* 
>     pendingSpecializedNative(NULL),
>     pendingUnboxSlot(NULL),
>     pendingGuardCondition(NULL),
>     pendingLoop(true),
>     generatedSpecializedNative(),
>     tempTypeMap(cx)
> {
>     JS_ASSERT(globalObj == cx->fp->scopeChain->getGlobal());
>+    JS_ASSERT(OBJ_SCOPE(globalObj)->hasOwnShape());

We talked also about how a global is created with null proto, so it has its own scope with a fresh shape (but not OWN_SHAPE). This is the emptyShape paired with the first property as key when growing the property tree, so first, ..., Nth sprops in the path from the root should all have fresh shapes that match no other global's sprops.

So I don't think we need to force OWN_SHAPE. But we should somehow assert that we don't need to (or that it's set due to branding, etc.).

Can you confirm the emptyShape induction above for shell and browser globals?

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 437658 [details] [diff] [review]
v2

Good for now. We need a bug on flattening scope into sprop (and obj where sprop presents a pigeon-hole problem).

Another bug on more categorical treatment of global objects and scopes would be good. Then we could prove facts such as uniquely-shaped more easily, maybe. Thoughts?

Thanks, r=me.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/c12e3fa489dc

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c12e3fa489dc