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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
I assume "aobj" isn't "array obj" here, because under no circumstances should we actually support an array object as a global.
(Assignee)

Comment 2

8 years ago
Created attachment 437414 [details] [diff] [review]
v1

No measurable improvement in SunSpider or V8. Still, a little less code is always a good thing.
Assignee: general → jorendorff
Attachment #437414 - Flags: review?(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
(Assignee)

Comment 4

8 years ago
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).
(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
(Assignee)

Comment 6

8 years ago
Created attachment 437658 [details] [diff] [review]
v2

Per discussion on IRC, insist on a unique shape for the global object before recording begins.
Attachment #437414 - Attachment is obsolete: true
Attachment #437658 - Flags: review?(brendan)
Attachment #437414 - Flags: review?(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 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
Attachment #437658 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

8 years ago
http://hg.mozilla.org/tracemonkey/rev/c12e3fa489dc
Summary: Don't redundantly guard(aobj != globalObj) → Eliminate redundant guard that incProp/getProp operand is not the global object
Whiteboard: fixed-in-tracemonkey

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c12e3fa489dc
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 566637
You need to log in before you can comment on or make changes to this bug.