Add a trace-time type to differentiate functions from objects

RESOLVED FIXED

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({fixed1.9.1})

unspecified
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

Results are inconclusive, but it seems I need at least a little perf to make up for bug 473117, and this would eliminate some guards and hopefully more than make up the difference.  Only question is whether it's a win or not, since it'll require more effort to differentiate on unbox on trace, but hopefully that's less common -- I'm thinking it may be, but I don't want to assume so until I have a working patch.
Shouldn't require more work to unbox, because both functions and objects have the same jsval tag and overhead. You'll want assertions, at most.

/be
Well, actually, it does; if you have a JSVAL_TAG(v) == JSVAL_OBJECT value that's being unboxed, you have to do something like this:

  if tag(v) == JSVAL_OBJECT):
    if v == JSVAL_NULL:
      guard(v_ins is 0)
    else if v is a function:
      guard(v_ins is not 0)
      guard(tag(v_ins) == JSVAL_OBJECT)
      guard(HAS_FUNCTION_CLASS(v_ins))
    else:
      guard(v_ins is not 0)
      guard(tag(v_ins) == JSVAL_OBJECT)
      guard(!HAS_FUNCTION_CLASS(v_ins))

I don't see how you can avoid this in general, since we unbox when getting a value from some entirely arbitrary place (fastnative return, for example) and can't use shapes or anything similar.

Patch complete, need to perf-test now...
The idea is to avoid that where we can with shape guards. Function-valued props induce branding and shape evolution when the function value changes. So you can be sure of not only function type, but value. But this is for a followup, namely bug 471214, which should depend on this bug.

/be
Posted patch JSVAL_TFUN (obsolete) — Splinter Review
This was actually pretty straightforward, all things considered -- JSVAL_TNULL pointed out the potholes.
Attachment #373389 - Flags: review?(brendan)
Attachment #373389 - Attachment is obsolete: true
Attachment #373410 - Flags: review?(brendan)
Attachment #373389 - Flags: review?(brendan)
Comment on attachment 373410 [details] [diff] [review]
With more similar get(Coerced|Promoted)Type method structure

> /* Return JSVAL_DOUBLE for all numbers (int and double) and the tag otherwise. */
> static inline uint8 getPromotedType(jsval v)
> {
>-    return JSVAL_IS_INT(v) ? JSVAL_DOUBLE : JSVAL_IS_NULL(v) ? JSVAL_TNULL : uint8(JSVAL_TAG(v));
>+    if (JSVAL_IS_INT(v))
>+        return JSVAL_DOUBLE;
>+    if (JSVAL_IS_OBJECT(v)) {
>+        if (JSVAL_IS_NULL(v))
>+            return JSVAL_TNULL;
>+        if (HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v)))
>+            return JSVAL_TFUN;
>+        return JSVAL_OBJECT;
>+    }
>+    return uint8(JSVAL_TAG(v));
> }
> 
> /* Return JSVAL_INT for all whole numbers that fit into signed 32-bit and the tag otherwise. */
> static inline uint8 getCoercedType(jsval v)
> {
>-    return isInt32(v) ? JSVAL_INT : JSVAL_IS_NULL(v) ? JSVAL_TNULL : uint8(JSVAL_TAG(v));
>+    if (isInt32(v))
>+        return JSVAL_INT;
>+    if (JSVAL_IS_OBJECT(v)) {
>+        if (JSVAL_IS_NULL(v))
>+            return JSVAL_TNULL;
>+        if (HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v)))
>+            return JSVAL_TFUN;
>+        return JSVAL_OBJECT;
>+    }
>+    return uint8(JSVAL_TAG(v));
> }

Nice, thanks.

> ValueToNative(JSContext* cx, jsval v, uint8 type, double* slot)
> {
>     unsigned tag = JSVAL_TAG(v);
>     switch (type) {
>+      case JSVAL_OBJECT:
>+        JS_ASSERT(tag == JSVAL_OBJECT);
>+        JS_ASSERT(!JSVAL_IS_NULL(v) && !HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v)));
>+        *(JSObject**)slot = JSVAL_TO_OBJECT(v);
>+        debug_only_v(printf("object<%p:%s> ", (void*)JSVAL_TO_OBJECT(v),
>+                            JSVAL_IS_NULL(v)
>+                            ? "null"
>+                            : STOBJ_GET_CLASS(JSVAL_TO_OBJECT(v))->name);)
>+        return;
>       case JSVAL_INT:
>         jsint i;
>         if (JSVAL_IS_INT(v))
>             *(jsint*)slot = JSVAL_TO_INT(v);
>         else if ((tag == JSVAL_DOUBLE) && JSDOUBLE_IS_INT(*JSVAL_TO_DOUBLE(v), i))
>             *(jsint*)slot = i;
>         else
>             JS_ASSERT(JSVAL_IS_INT(v));
>@@ -1494,42 +1524,51 @@ ValueToNative(JSContext* cx, jsval v, ui
>         if (JSVAL_IS_INT(v))
>             d = JSVAL_TO_INT(v);
>         else
>             d = *JSVAL_TO_DOUBLE(v);
>         JS_ASSERT(JSVAL_IS_INT(v) || JSVAL_IS_DOUBLE(v));
>         *(jsdouble*)slot = d;
>         debug_only_v(printf("double<%g> ", d);)
>         return;
>+      case JSVAL_BOXED:
>+        JS_NOT_REACHED("found boxed type in an entry type map");
>+        return;
>+      case JSVAL_STRING:
>+        JS_ASSERT(tag == JSVAL_STRING);
>+        *(JSString**)slot = JSVAL_TO_STRING(v);
>+        debug_only_v(printf("string<%p> ", (void*)(*(JSString**)slot));)
>+        return;
>+      case JSVAL_TNULL:
>+        JS_ASSERT(tag == JSVAL_OBJECT);
>+        *(JSObject**)slot = NULL;
>+        debug_only_v(printf("null ");)
>+        return;
>       case JSVAL_BOOLEAN:
>         /* Watch out for pseudo-booleans. */
>         JS_ASSERT(tag == JSVAL_BOOLEAN);
>         *(JSBool*)slot = JSVAL_TO_PSEUDO_BOOLEAN(v);
>         debug_only_v(printf("boolean<%d> ", *(JSBool*)slot);)
>         return;
>-      case JSVAL_STRING:
>-        JS_ASSERT(tag == JSVAL_STRING);
>-        *(JSString**)slot = JSVAL_TO_STRING(v);
>-        debug_only_v(printf("string<%p> ", (void*)(*(JSString**)slot));)
>-        return;
>-      case JSVAL_TNULL:
>+      case JSVAL_TFUN: {
>         JS_ASSERT(tag == JSVAL_OBJECT);
>-        *(JSObject**)slot = NULL;
>-        return;
>-      default:
>-        /* Note: we should never see JSVAL_BOXED in an entry type map. */
>-        JS_ASSERT(type == JSVAL_OBJECT);
>-        JS_ASSERT(tag == JSVAL_OBJECT);
>-        *(JSObject**)slot = JSVAL_TO_OBJECT(v);
>-        debug_only_v(printf("object<%p:%s> ", (void*)JSVAL_TO_OBJECT(v),
>-                            JSVAL_IS_NULL(v)
>-                            ? "null"
>-                            : STOBJ_GET_CLASS(JSVAL_TO_OBJECT(v))->name);)
>-        return;
>-    }
>+        JSObject* obj = JSVAL_TO_OBJECT(v);
>+        *(JSObject**)slot = obj;
>+#ifdef DEBUG
>+        JSFunction* fun = GET_FUNCTION_PRIVATE(cx, obj);
>+        debug_only_v(printf("function<%p:%s> ", (void*) obj,
>+                            fun->atom
>+                            ? JS_GetStringBytes(ATOM_TO_STRING(fun->atom))
>+                            : "unnamed");)
>+#endif
>+        return;
>+      }
>+    }
>+
>+    JS_NOT_REACHED("unexpected type");

Would prefer minimized change above -- the default: case for object type is no less safe than a compiled-to-nothing JS_NOT_REACHED followed by an early return if JSVAL_BOXED ever happened in the field. We need debug-build test coverage at most here.

>@@ -1609,18 +1648,20 @@ NativeToValue(JSContext* cx, jsval& v, u
>         d = (jsdouble)i;
>         goto store_double;
>       case JSVAL_DOUBLE:
>         d = *slot;
>         debug_only_v(printf("double<%g> ", d);)
>         if (JSDOUBLE_IS_INT(d, i))
>             goto store_int;
>       store_double: {
>-        /* Its not safe to trigger the GC here, so use an emergency heap if we are out of
>-           double boxes. */
>+        /*
>+         * It's not safe to trigger the GC here, so use an emergency heap if we
>+         * are out of double boxes.
>+         */

Much as I prefer K&R comment style it seems unnecessary in this patch.

>@@ -1639,16 +1680,28 @@ NativeToValue(JSContext* cx, jsval& v, u
>         JS_ASSERT(v != JSVAL_ERROR_COOKIE); /* don't leak JSVAL_ERROR_COOKIE */
>         debug_only_v(printf("box<%p> ", (void*)v));
>         break;
>       case JSVAL_TNULL:
>         JS_ASSERT(*(JSObject**)slot == NULL);
>         v = JSVAL_NULL;
>         debug_only_v(printf("null<%p> ", (void*)(*(JSObject**)slot)));
>         break;
>+      case JSVAL_TFUN: {
>+        JS_ASSERT(HAS_FUNCTION_CLASS(*(JSObject**)slot));
>+        v = OBJECT_TO_JSVAL(*(JSObject**)slot);
>+#ifdef DEBUG
>+        JSFunction* fun = GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(v));
>+        debug_only_v(printf("function<%p:%s> ", (void*)JSVAL_TO_OBJECT(v),
>+                            fun->atom
>+                            ? JS_GetStringBytes(ATOM_TO_STRING(fun->atom))
>+                            : "unnamed");)
>+#endif
>+        break;
>+      }
>       default:
>         JS_ASSERT(type == JSVAL_OBJECT);
>         v = OBJECT_TO_JSVAL(*(JSObject**)slot);
>         JS_ASSERT(JSVAL_TAG(v) == JSVAL_OBJECT); /* if this fails the pointer was not aligned */
>         JS_ASSERT(v != JSVAL_ERROR_COOKIE); /* don't leak JSVAL_ERROR_COOKIE */
>         debug_only_v(printf("object<%p:%s> ", (void*)JSVAL_TO_OBJECT(v),
>                             JSVAL_IS_NULL(v)
>                             ? "null"

Observing that the above minimized change, unlike earlier or (below) later.

> JS_REQUIRES_STACK uint8
> TraceRecorder::determineSlotType(jsval* vp)
> {
>     uint8 m;
>     LIns* i = get(vp);
>-    m = isNumber(*vp)
>-        ? (isPromoteInt(i) ? JSVAL_INT : JSVAL_DOUBLE)
>-        : JSVAL_IS_NULL(*vp)
>-        ? JSVAL_TNULL
>-        : JSVAL_TAG(*vp);
>+    if (isNumber(*vp))
>+        m = isPromoteInt(i) ? JSVAL_INT : JSVAL_DOUBLE;
>+    else if (JSVAL_IS_NULL(*vp))
>+        m = JSVAL_TNULL;
>+    else if (JSVAL_IS_PRIMITIVE(*vp))
>+        m = JSVAL_TAG(*vp);
>+    else if (HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(*vp)))
>+        m = JSVAL_TFUN;
>+    else
>+        m = JSVAL_OBJECT;

Use same pattern as earlier for minimal JSVAL_IS_NULL and to reinforce the pattern. If it's good before it's good here.

>+    if (t == JSVAL_TFUN)
>+        return !JSVAL_IS_PRIMITIVE(v) && HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v));
>+    if (t == JSVAL_OBJECT)
>+        return !JSVAL_IS_PRIMITIVE(v) && !HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v));

Blank line here?

> static bool
> js_IsEntryTypeCompatible(jsval* vp, uint8* m)
> {
>     unsigned tag = JSVAL_TAG(*vp);
> 
>     debug_only_v(printf("%c/%c ", tagChar[tag], typeChar[*m]);)
> 
>     switch (*m) {
>+      case JSVAL_OBJECT:
>+        if (tag == JSVAL_OBJECT && !JSVAL_IS_NULL(*vp) &&
>+            !HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(*vp))) {
>+            return true;
>+        }
>+        debug_only_v(printf("object != tag%u", tag);)
>+        return false;
>       case JSVAL_INT:
>         jsint i;
>         if (JSVAL_IS_INT(*vp))
>             return true;
>         if ((tag == JSVAL_DOUBLE) && JSDOUBLE_IS_INT(*JSVAL_TO_DOUBLE(*vp), i))
>             return true;
>         debug_only_v(printf("int != tag%u(value=%lu) ", tag, (unsigned long)*vp);)
>         return false;
>       case JSVAL_DOUBLE:
>         if (JSVAL_IS_INT(*vp) || tag == JSVAL_DOUBLE)
>             return true;
>         debug_only_v(printf("double != tag%u ", tag);)
>         return false;
>+      case JSVAL_BOXED:
>+        JS_NOT_REACHED("shouldn't see boxed type in entry");
>+        return false;
>+      case JSVAL_STRING:
>+        if (tag == JSVAL_STRING)
>+            return true;
>+        debug_only_v(printf("string != tag%u", tag);)
>+        return false;
>+      case JSVAL_TNULL:
>+        if (JSVAL_IS_NULL(*vp))
>+            return true;
>+        debug_only_v(printf("null != tag%u", tag);)
>+        return false;
>       case JSVAL_BOOLEAN:
>         if (tag == JSVAL_BOOLEAN)
>             return true;
>         debug_only_v(printf("bool != tag%u", tag);)
>         return false;
>-      case JSVAL_STRING:
>-        if (tag == JSVAL_STRING)
>-            return true;
>-        debug_only_v(printf("string != tag%u", tag);)
>-        return false;
>-      case JSVAL_TNULL:
>-        return JSVAL_IS_NULL(*vp);
>-      default:
>-        JS_ASSERT(*m == JSVAL_OBJECT);
>-        if (tag == JSVAL_OBJECT && !JSVAL_IS_NULL(*vp))
>-            return true;
>-        debug_only_v(printf("object != tag%u", tag);)
>-        return false;
>-    }
>+      case JSVAL_TFUN:
>+        if (tag == JSVAL_OBJECT && !JSVAL_IS_NULL(*vp) &&
>+            HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(*vp))) {
>+            return true;
>+        }
>+        debug_only_v(printf("fun != tag%u", tag);)
>+        return false;
>+    }
>+
>+    JS_NOT_REACHED("bad entry type");
>+    return false;
> }

The above is non-minimal but it does defend a bit by returning false if, somehow, JSVAL_BOXED appears. 

This suggests the earlier ValueToNative code should defend similarly.

>@@ -6191,24 +6274,48 @@ TraceRecorder::unbox_jsval(jsval v, LIns
>                          JSVAL_BOOLEAN),
>               exit);
>         v_ins = lir->ins2i(LIR_ush, v_ins, JSVAL_TAGBITS);
>         return;
>       case JSVAL_OBJECT:
>         if (JSVAL_IS_NULL(v)) {
>             // JSVAL_NULL maps to type JSVAL_TNULL, so insist that v_ins == 0 here.
>             guard(true, lir->ins_eq0(v_ins), exit);
>-        } else {
>-            // We must guard that v_ins has JSVAL_OBJECT tag but is not JSVAL_NULL.
>+        } else if (HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v))) {
>+            guard(false, lir->ins_eq0(v_ins), exit);
>             guard(true,
>                   lir->ins2i(LIR_eq,
>-                             lir->ins2(LIR_piand, v_ins, INS_CONST(JSVAL_TAGMASK)),
>+                             lir->ins2(LIR_piand, v_ins, INS_CONSTPTR((void*)JSVAL_TAGMASK)),
>                              JSVAL_OBJECT),
>                   exit);
>+            guard(true,
>+                  lir->ins2(LIR_eq,
>+                            lir->ins2(LIR_piand,
>+                                      lir->insLoad(LIR_ldp, v_ins, offsetof(JSObject, classword)),
>+                                      INS_CONSTPTR((void*)~JSSLOT_CLASS_MASK_BITS)),
>+                            INS_CONSTPTR(&js_FunctionClass)),
>+                  exit);
>+        } else {
>+            /*
>+             * We must guard that v_ins has JSVAL_OBJECT tag but is neither null
>+             * nor a function.
>+             */
>             guard(false, lir->ins_eq0(v_ins), exit);
>+            guard(true,
>+                  lir->ins2i(LIR_eq,
>+                             lir->ins2(LIR_piand, v_ins, INS_CONSTPTR((void*)JSVAL_TAGMASK)),
>+                             JSVAL_OBJECT),
>+                  exit);
>+            guard(false,
>+                  lir->ins2(LIR_eq,
>+                            lir->ins2(LIR_piand,
>+                                      lir->insLoad(LIR_ldp, v_ins, offsetof(JSObject, classword)),
>+                                      INS_CONSTPTR((void*)~JSSLOT_CLASS_MASK_BITS)),
>+                            INS_CONSTPTR(&js_FunctionClass)),
>+                  exit);
>         }

The else-if and final else consequents are the same code with the final guard call's leading argument (true, false) synthesized from HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(v)). Combine!

>@@ -7849,24 +7956,16 @@ TraceRecorder::guardCallee(jsval& callee
> 
>     VMSideExit* branchExit = snapshot(BRANCH_EXIT);
>     JSObject* callee_obj = JSVAL_TO_OBJECT(callee);
>     LIns* callee_ins = get(&callee);
> 
>     guard(true,
>           lir->ins2(LIR_eq,
>                     lir->ins2(LIR_piand,
>-                              lir->insLoad(LIR_ldp, callee_ins,
>-                                           offsetof(JSObject, classword)),
>-                              INS_CONSTPTR((void*)(~JSSLOT_CLASS_MASK_BITS))),
>-                    INS_CONSTPTR(&js_FunctionClass)),
>-          snapshot(MISMATCH_EXIT));
>-    guard(true,
>-          lir->ins2(LIR_eq,
>-                    lir->ins2(LIR_piand,
>                               stobj_get_fslot(callee_ins, JSSLOT_PRIVATE),
>                               INS_CONSTPTR((void*)(~JSVAL_INT))),
>                     INS_CONSTPTR(OBJ_GET_PRIVATE(cx, callee_obj))),
>           branchExit);
>     guard(true,
>           lir->ins2(LIR_eq,
>                     stobj_get_fslot(callee_ins, JSSLOT_PARENT),
>                     INS_CONSTPTR(OBJ_GET_PARENT(cx, callee_obj))),

This is the winning bit ;-).

/be
I find it difficult to establish full coverage of all potential cases in a type or tag switch unless they are in the same numeric order as the types/tags themselves, i.e. exactly this order: JSVAL_OBJECT, JSVAL_INT, JSVAL_DOUBLE, JSVAL_BOXED, JSVAL_STRING, JSVAL_TNULL, JSVAL_BOOLEAN, JSVAL_TFUN.  If I don't have this rearrangement, I find myself mentally going through the tags in exactly this order, but I have to skip around within the switch to make sure every one is covered -- and woe if a case like JSVAL_BOXED is lumped in with JSVAL_OBJECT, because I have to do a linear search to really verify that, then return back to the default to see that an assertion is hit.  I've split the reordering into a forthcoming patch atop this one, to be attached momentarily.
Attachment #373410 - Attachment is obsolete: true
Attachment #373433 - Flags: review?(brendan)
Attachment #373410 - Flags: review?(brendan)
If we take no prisoners, we need this to land bug 473117, which blocks...
Flags: blocking1.9.1?
Attachment #373434 - Flags: review?(gal) → review+
Comment on attachment 373433 [details] [diff] [review]
Without readability-enhancing rearrangements

Looks great. And thanks for splitting the patch.

/be
Attachment #373433 - Flags: review?(brendan) → review+
(In reply to comment #9)
> If we take no prisoners, we need this to land bug 473117, which blocks...

can you post performance numbers?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
I did several 10x runs, a couple 20x runs, and two 100x runs comparing without 473117 and with.  I didn't see any runs where 473117 was faster than not; the impact on individual tests was that some were faster, some weren't.  The two 100x runs, naturally, one would expect to mostly have similar numbers, but they didn't.  One had an overall 1.3% slowdown, while the other had a 0.6% slowdown; on my system a full run is about 2400ms, and I saw 40ms differences between the differences between with-473117 and without.  I have no idea why results would be so inconsistent, but it seemed better not to mess around and just to fix this -- pristine compared to 473117+everything here was about a 1% speedup.

Fixed in TM:

http://hg.mozilla.org/tracemonkey/rev/c59fd9494490
http://hg.mozilla.org/tracemonkey/rev/6ab042310fb4
Whiteboard: fixed-in-tracemonkey
Oh, if I had the exact stats I'd post them, but I had a system crash while that terminal was still open, and I lost them.  Could regen, but it's a bit of a pain to do so, all things considered.
Duplicate of this bug: 481273
http://hg.mozilla.org/mozilla-central/rev/e8588a4a1153
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.