497948 - clean up deduceTypeStability

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

These functions are really messy and they all do the same thing: compare two typemaps. I'm working on a patch to generalize this so the code is readable and the logic is in one place.

This has come up because recursion once again duplicates this logic, and additionally there is a bug in deduceTypeStability where the demote outparam is never set. This will become a separate bug, here I will keep the broken functionality intact.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP

WIP, so far preserves functionality. Andreas suggested immediately performing demotions - this I will do separately once all the changes are done.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v2

Comment 3

[Graydon Hoare :graydon]

We are colliding somewhat hard here, see bug 496674?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #3)
> We are colliding somewhat hard here, see bug 496674?

You go first! It will give me a chance to clean this up a little more.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v3

New version against visitor refactoring, took a new approach based on recursion use case.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: final patch

fixes regression in previous patch. functionality is identical to the old code now, just with ~200 less lines of code. the major change is that closeLoop doesn't need to depend on the tracker, since for up-recursion tracker info isn't available in closeLoop.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

there was also one output change, the type checking process is more verbose and consistent:

checkType slot 0: interp=F typemap=F isNum=0 promoteInt=0
checkType slot 1: interp=N typemap=N isNum=0 promoteInt=0
checkType slot 2: interp=I typemap=I isNum=1 promoteInt=1
checkType slot 3: interp=I typemap=I isNum=1 promoteInt=1
checkType slot 4: interp=O typemap=O isNum=0 promoteInt=0

Comment 8

[Andreas Gal :gal]

Comment on attachment 385891 [details] [diff] [review]
final patch

>+/* Results of trying to connect an arbitrary type A with arbitrary type B */
>+enum TypeCheckResult
>+{
>+    TypeCheck_Okay,         /* Okay: same type */
>+    TypeCheck_Promote,      /* Okay: Type A needs f2i() */
>+    TypeCheck_Demote,       /* Okay: Type A needs i2f() */
>+    TypeCheck_Undemote,     /* Bad: Slot is undemotable */
>+    TypeCheck_Bad           /* Bad: incompatible types */
>+};
>+
>+class SlotMap : public SlotVisitorBase
>+{
>+public:

Indent public: label by 2, according to the style police chief.

>+    struct SlotInfo
>+    {
>+        SlotInfo(jsval* v, bool promoteInt) :
>+            v(v), promoteInt(promoteInt), lastCheck(TypeCheck_Bad)

: in the next line, also indented by 2.

>+        {
>+        }

new line

>+        jsval           *v;
>+        bool            promoteInt;
>+        TypeCheckResult lastCheck;
>+    };
>+
>+    SlotMap(TraceRecorder& rec, bool useTracker) :
>+        mRecorder(rec), mCx(rec.cx), useTracker(useTracker)

:

>+    {
>+    }
>+
>+    JS_REQUIRES_STACK JS_ALWAYS_INLINE bool
>+    visitStackSlots(jsval *vp, size_t count, JSStackFrame* fp)
>+    {
>+        for (size_t i = 0; i < count; i++)
>+            addSlot(&vp[i]);
>+        return true;
>+    }
>+
>+    JS_REQUIRES_STACK JS_ALWAYS_INLINE void
>+    visitGlobalSlot(jsval *vp, unsigned n, unsigned slot)
>+    {
>+        addSlot(vp);
>+    }
>+
>+    JS_ALWAYS_INLINE SlotMap::SlotInfo&
>+    operator [](unsigned i)
>+    {
>+        return slots[i];
>+    }
>+
>+    JS_ALWAYS_INLINE SlotMap::SlotInfo&
>+    get(unsigned i)
>+    {
>+        return slots[i];
>+    }
>+
>+    JS_ALWAYS_INLINE unsigned
>+    length()
>+    {
>+        return slots.length();
>+    }
>+
>+    TypeConsensus
>+    checkTypes(TreeInfo* ti)
>+    {
>+        if (length() != ti->typeMap.length())
>+            return TypeConsensus_Bad;
>+
>+        TypeCheckResult result;
>+        bool has_undemotes = false;
>+        for (unsigned i = 0; i < length(); i++) {
>+            result = checkType(i, ti->typeMap[i]);

Move definition of result here?

>+            if (result == TypeCheck_Bad)
>+                return TypeConsensus_Bad;
>+            if (result == TypeCheck_Undemote)
>+                has_undemotes = true;
>+            slots[i].lastCheck = result;
>+        }
>+        if (has_undemotes)
>+            return TypeConsensus_Undemotes;
>+        return TypeConsensus_Okay;
>+    }
>+private:

indent by 2

>+    JS_REQUIRES_STACK JS_ALWAYS_INLINE void
>+    addSlot(jsval* vp)
>+    {
>+        bool promote = false;
>+        if (isNumber(*vp) &&
>+            ((useTracker && isPromoteInt(mRecorder.get(vp))) || (!useTracker && isInt32(*vp)))) {
>+            promote = true;
>+        }
>+        slots.add(SlotInfo(vp, promote));
>+    }
>+
>+    TypeCheckResult
>+    checkType(unsigned i, JSTraceType t)
>+    {
>+        debug_only_printf(LC_TMTracer,
>+                          "checkType slot %d: interp=%c typemap=%c isNum=%d promoteInt=%d\n", 
>+                          i,
>+                          typeChar[getCoercedType(*slots[i].v)],
>+                          typeChar[t],
>+                          isNumber(*slots[i].v),
>+                          slots[i].promoteInt);

gcc called. It has switch statements now.

>+        if (t == TT_INT32) {
>+            if (!isNumber(*slots[i].v))
>+                return TypeCheck_Bad; /* Not a number? Type mismatch. */
>+            /* This is always a type mismatch, we can't close a double to an int. */
>+            if (!slots[i].promoteInt)
>+                return TypeCheck_Undemote;
>+            /* Looks good, slot is an int32, the last instruction should be promotable. */
>+            JS_ASSERT(isInt32(*slots[i].v) && slots[i].promoteInt);
>+            return TypeCheck_Promote;
>+        } else if (t == TT_DOUBLE) {
>+            if (!isNumber(*slots[i].v))
>+                return TypeCheck_Bad; /* Not a number? Type mismatch. */
>+            if (slots[i].promoteInt)
>+                return TypeCheck_Demote;
>+            return TypeCheck_Okay;
>+        } else if (t == TT_NULL) {
>+            return JSVAL_IS_NULL(*slots[i].v) ? TypeCheck_Okay : TypeCheck_Bad;
>+        } else if (t == TT_FUNCTION) {
>+            return !JSVAL_IS_PRIMITIVE(*slots[i].v) &&
>+                   HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(*slots[i].v)) ?
>+                   TypeCheck_Okay : TypeCheck_Bad;
>+        } else if (t == TT_OBJECT) {
>+            return !JSVAL_IS_PRIMITIVE(*slots[i].v) &&
>+                   !HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(*slots[i].v)) ?
>+                   TypeCheck_Okay : TypeCheck_Bad;
>+        }
>+        return getCoercedType(*slots[i].v) == t ? TypeCheck_Okay : TypeCheck_Bad;
>+    }
>+private:

Indent.

>+    TraceRecorder& mRecorder;
>+    JSContext* mCx;
>+    bool       useTracker;
>+    Queue<SlotInfo> slots;
>+};
>+
>+JS_REQUIRES_STACK TypeConsensus
>+TraceRecorder::deduceTypeStability(SlotMap& smap, VMFragment** pPeer)
>+{
>+    bool onlyUndemotes = false;
>+
>+    *pPeer = NULL;
>+    debug_only_printf(LC_TMTracer, "Checking type stability against self=%p\n", fragment);
>+    TypeConsensus consensus = smap.checkTypes(treeInfo);
>+
>+    /* Best case: loop can be self-closed */

Best case: we can loop back to our own loop header

>+    if (consensus == TypeConsensus_Okay)
>+        return TypeConsensus_Okay;
>+
>+    /* Almost self-closed, make a note */
>+    if (consensus == TypeConsensus_Undemotes)
>+    {
>+        for (unsigned i = 0; i < smap.length(); i++)
>+        {
>+            if (smap[i].lastCheck == TypeCheck_Undemote)
>+                markSlotUndemotable(cx, treeInfo, i);
>+        }
>+        onlyUndemotes = true;
>+    }
>+
>+    /* See if there are any peers that would make this stable */
>+    VMFragment* root = (VMFragment*)fragment->root;
>+    VMFragment* peer = getLoop(traceMonitor, root->ip, root->globalObj, root->globalShape,
>+                               root->argc);
>+    JS_ASSERT(peer != NULL);
>+    for (; peer != NULL; peer = (VMFragment*)peer->peer) {
>+        if (!peer->vmprivate || peer == fragment)
>+            continue;
>+        debug_only_printf(LC_TMTracer, "Checking type stability against peer=%p\n", peer);
>+        consensus = smap.checkTypes((TreeInfo*)peer->vmprivate);
>+        if (consensus == TypeConsensus_Okay) {
>+            *pPeer = peer;
>+            /* Return this even though there will be linkage; the trace itself is not stable. */
>+            return TypeConsensus_Bad;
>+        }
>+        if (consensus == TypeConsensus_Undemotes)
>+            onlyUndemotes = true;
>+    }
>+
>+    return onlyUndemotes ? TypeConsensus_Undemotes : TypeConsensus_Bad;
>+}
>+
>+JS_REQUIRES_STACK bool
>+TraceRecorder::closeLoop(TypeConsensus &consensus)
>+{
>+    SlotMap slotMap(*this, true);
>+    VisitSlots(slotMap, cx, 0, *treeInfo->globalSlots);
>+    return closeLoop(&slotMap, consensus);
>+}
>+
>+/* Complete and compile a trace and link it to the existing tree if appropriate.
>+ * Returns true if something was compiled. Outparam is always set.
>+ */
>+JS_REQUIRES_STACK bool
>+TraceRecorder::closeLoop(SlotMap* slotMap, TypeConsensus& consensus)
> {
>     /*
>      * We should have arrived back at the loop header, and hence we don't want to be in an imacro
>@@ -3540,44 +3371,50 @@
>      */
>     JS_ASSERT((*cx->fp->regs->pc == JSOP_LOOP || *cx->fp->regs->pc == JSOP_NOP) && !cx->fp->imacpc);
> 
>-    bool stable;
>-    Fragment* peer;
>-    VMFragment* peer_root;
>-    Fragmento* fragmento = tm->fragmento;
>+    Fragmento* fragmento = traceMonitor->fragmento;
> 
>     if (callDepth != 0) {
>         debug_only_print0(LC_TMTracer,
>                           "Blacklisted: stack depth mismatch, possible recursion.\n");
>         js_Blacklist((jsbytecode*) fragment->root->ip);
>         trashSelf = true;
>-        return;
>+        consensus = TypeConsensus_Bad;
>+        return false;
>     }
> 
>     VMSideExit* exit = snapshot(UNSTABLE_LOOP_EXIT);
>     JS_ASSERT(exit->numStackSlots == treeInfo->nStackTypes);
> 
>+    VMFragment* peer;
>     VMFragment* root = (VMFragment*)fragment->root;
>-    peer_root = getLoop(traceMonitor, root->ip, root->globalObj, root->globalShape, root->argc);
>-    JS_ASSERT(peer_root != NULL);
>-
>-    stable = deduceTypeStability(peer_root, &peer, demote);
>+
>+    consensus = deduceTypeStability(*slotMap, &peer);
> 
> #if DEBUG
>-    if (!stable)
>+    if (consensus != TypeConsensus_Okay)
>         AUDIT(unstableLoopVariable);
> #endif
> 
>-    if (trashSelf) {
>-        debug_only_print0(LC_TMTracer, "Trashing tree from type instability.\n");
>-        return;
>-    }
>-
>-    if (stable && demote) {
>-        JS_ASSERT(fragment->kind == LoopTrace);
>-        return;
>-    }
>-
>-    if (!stable) {
>+    JS_ASSERT(!trashSelf);
>+
>+    /* This exit is indeed linkable to something now. Process any promote/demotes that
>+     * are pending in the slot map.
>+     */
>+    if (consensus == TypeConsensus_Okay || peer) {
>+        for (unsigned i = 0; i < slotMap->length(); i++) {
>+            SlotMap::SlotInfo& info = slotMap->get(i);
>+            JS_ASSERT(info.lastCheck != TypeCheck_Undemote && info.lastCheck != TypeCheck_Bad);
>+            if (info.lastCheck == TypeCheck_Promote) {
>+                JS_ASSERT(isNumber(*info.v));
>+                set(info.v, f2i(get(info.v)));
>+            } else if (info.lastCheck == TypeCheck_Demote) {
>+                JS_ASSERT(isNumber(*info.v));
>+                set(info.v, lir->ins1(LIR_i2f, get(info.v)));
>+            }
>+        }
>+    }
>+
>+    if (consensus != TypeConsensus_Okay) {
>         fragment->lastIns = lir->insGuard(LIR_x, lir->insImm(1), createGuardRecord(exit));
> 
>         /*
>@@ -3604,7 +3441,6 @@
>             debug_only_printf(LC_TMTracer,
>                               "Joining type-unstable trace to target fragment %p.\n",
>                               (void*)peer);
>-            stable = true;
>             ((TreeInfo*)peer->vmprivate)->dependentTrees.addUnique(fragment->root);
>             treeInfo->linkedTrees.addUnique(peer);
>         }
>@@ -3612,12 +3448,14 @@
>         exit->target = fragment->root;
>         fragment->lastIns = lir->insGuard(LIR_loop, lir->insImm(1), createGuardRecord(exit));
>     }
>-    compile(tm);
>+    compile(traceMonitor);
> 
>     if (fragmento->assm()->error() != nanojit::None)
>-        return;
>-
>-    joinEdgesToEntry(fragmento, peer_root);
>+        return false;
>+
>+    peer = getLoop(traceMonitor, root->ip, root->globalObj, root->globalShape, root->argc);
>+    JS_ASSERT(peer != NULL);
>+    joinEdgesToEntry(fragmento, peer);
> 
>     debug_only_print0(LC_TMTracer,
>                       "updating specializations on dependent and linked trees\n");
>@@ -3629,7 +3467,7 @@
>      * should try to compile the outer tree again.
>      */
>     if (outer)
>-        js_AttemptCompilation(cx, tm, globalObj, outer, outerArgc);
>+        js_AttemptCompilation(cx, traceMonitor, globalObj, outer, outerArgc);
> #ifdef JS_JIT_SPEW
>     debug_only_printf(LC_TMMinimal,
>                       "recording completed at  %s:%u@%u via closeLoop\n",
>@@ -3638,6 +3476,8 @@
>                       FramePCOffset(cx->fp));
>     debug_only_print0(LC_TMMinimal, "\n");
> #endif
>+
>+    return true;
> }
> 
> JS_REQUIRES_STACK void
>@@ -3933,20 +3773,10 @@
>             cx->fp->regs->pc = (jsbytecode*)fragment->root->ip;
>             cx->fp->regs->sp -= fused ? 2 : 1;
> 
>-            bool demote = false;
>-            closeLoop(traceMonitor, demote);
>+            TypeConsensus consensus;
>+            closeLoop(consensus);
> 
>             *cx->fp->regs = orig;
>-
>-            /*
>-             * If compiling this loop generated new oracle information which will likely
>-             * lead to a different compilation result, immediately trigger another
>-             * compiler run. This is guaranteed to converge since the oracle only
>-             * accumulates adverse information but never drops it (except when we
>-             * flush it during garbage collection.)
>-             */
>-            if (demote)
>-                js_AttemptCompilation(cx, traceMonitor, globalObj, outer, outerArgc);
>         } else {
>             endLoop(traceMonitor);
>         }
>@@ -4443,18 +4273,6 @@
>     return true;
> }
> 
>-JS_REQUIRES_STACK static inline void
>-markSlotUndemotable(JSContext* cx, TreeInfo* ti, unsigned slot)
>-{
>-    if (slot < ti->nStackTypes) {
>-        oracle.markStackSlotUndemotable(cx, slot);
>-        return;
>-    }
>-
>-    uint16* gslots = ti->globalSlots->data();
>-    oracle.markGlobalSlotUndemotable(cx, gslots[slot - ti->nStackTypes]);
>-}
>-
> JS_REQUIRES_STACK static inline bool
> isSlotUndemotable(JSContext* cx, TreeInfo* ti, unsigned slot)
> {
>diff -r 71e3e7b40341 js/src/jstracer.h
>--- a/js/src/jstracer.h	Fri Jun 26 16:29:38 2009 -0700
>+++ b/js/src/jstracer.h	Mon Jun 29 16:08:56 2009 -0700
>@@ -120,11 +120,20 @@
>         _len = 0;
>     }
> 
>+    T & get(unsigned i) {
>+        JS_ASSERT(i < length());
>+        return _data[i];
>+    }
>+
>     const T & get(unsigned i) const {
>         JS_ASSERT(i < length());
>         return _data[i];
>     }
> 
>+    T & operator [](unsigned i) {
>+        return get(i);
>+    }
>+
>     const T & operator [](unsigned i) const {
>         return get(i);
>     }
>@@ -538,7 +547,15 @@
> #define STATUS_ABORTS_RECORDING(s) ((s) <= JSRS_STOP)
> #endif
> 
>+class SlotMap;
> 
>+/* Results of trying to compare two typemaps together */
>+enum TypeConsensus
>+{
>+    TypeConsensus_Okay,         /* Two typemaps are compatible */
>+    TypeConsensus_Undemotes,    /* Not compatible now, but would be with pending undemotes. */
>+    TypeConsensus_Bad           /* Typemaps are not compatible */
>+};
> 
> class TraceRecorder : public avmplus::GCObject {
>     JSContext*              cx;
>@@ -604,11 +621,7 @@
>     JS_REQUIRES_STACK bool known(jsval* p);
>     JS_REQUIRES_STACK void checkForGlobalObjectReallocation();
> 
>-    JS_REQUIRES_STACK bool checkType(jsval& v, JSTraceType t, jsval*& stage_val,
>-                                     nanojit::LIns*& stage_ins, unsigned& stage_count);
>-    JS_REQUIRES_STACK bool deduceTypeStability(nanojit::Fragment* root_peer,
>-                                               nanojit::Fragment** stable_peer,
>-                                               bool& demote);
>+    JS_REQUIRES_STACK TypeConsensus deduceTypeStability(SlotMap& smap, VMFragment** peer);
> 
>     JS_REQUIRES_STACK jsval& argval(unsigned n) const;
>     JS_REQUIRES_STACK jsval& varval(unsigned n) const;
>@@ -774,7 +787,8 @@
>     nanojit::Fragment* getFragment() const { return fragment; }
>     TreeInfo* getTreeInfo() const { return treeInfo; }
>     JS_REQUIRES_STACK void compile(JSTraceMonitor* tm);
>-    JS_REQUIRES_STACK void closeLoop(JSTraceMonitor* tm, bool& demote);
>+    JS_REQUIRES_STACK bool closeLoop(TypeConsensus &consensus);
>+    JS_REQUIRES_STACK bool closeLoop(SlotMap* slotMap, TypeConsensus &consensus);
>     JS_REQUIRES_STACK void endLoop(JSTraceMonitor* tm);
>     JS_REQUIRES_STACK void joinEdgesToEntry(nanojit::Fragmento* fragmento,
>                                             VMFragment* peer_root);
>@@ -810,9 +824,7 @@
>     friend class AdjustCallerGlobalTypesVisitor;
>     friend class AdjustCallerStackTypesVisitor;
>     friend class TypeCompatibilityVisitor;
>-    friend class SelfTypeStabilityVisitor;
>-    friend class PeerTypeStabilityVisitor;
>-    friend class UndemoteVisitor;
>+    friend class SlotMap;
> };
> #define TRACING_ENABLED(cx)       JS_HAS_OPTION(cx, JSOPTION_JIT)
> #define TRACE_RECORDER(cx)        (JS_TRACE_MONITOR(cx).recorder)

Comment 9

[Robert Sayre]

drive by organization nits:

>+JS_REQUIRES_STACK TypeConsensus
>+TraceRecorder::deduceTypeStability(SlotMap& smap, VMFragment** pPeer)
>+{
>+    bool onlyUndemotes = false;

This is used to track something across this big long function body. Need to break this up.

>+    /* Almost self-closed, make a note */
>+    if (consensus == TypeConsensus_Undemotes)
>+    {
>+        for (unsigned i = 0; i < smap.length(); i++)
>+        {
>+            if (smap[i].lastCheck == TypeCheck_Undemote)
>+                markSlotUndemotable(cx, treeInfo, i);
>+        }
>+        onlyUndemotes = true;
>+    }
>+

^^ here's one bit 

bool onlyUndemotes = (consensus == TypeConsensus_Undemotes);

if (onlyUndemotes)
    markSlotMapUndemotables(...);
    
The second part below should be a function too...

>+    /* See if there are any peers that would make this stable */
>+    VMFragment* root = (VMFragment*)fragment->root;
>+    VMFragment* peer = getLoop(traceMonitor, root->ip, root->globalObj, root->globalShape,
>+                               root->argc);
>+    JS_ASSERT(peer != NULL);
>+    for (; peer != NULL; peer = (VMFragment*)peer->peer) {
>+        if (!peer->vmprivate || peer == fragment)
>+            continue;
>+        debug_only_printf(LC_TMTracer, "Checking type stability against peer=%p\n", peer);
>+        consensus = smap.checkTypes((TreeInfo*)peer->vmprivate);
>+        if (consensus == TypeConsensus_Okay) {
>+            *pPeer = peer;
>+            /* Return this even though there will be linkage; the trace itself is not stable. */
>+            return TypeConsensus_Bad;
>+        }
>+        if (consensus == TypeConsensus_Undemotes)
>+            onlyUndemotes = true;
>+    }

... onlyUndemotes = checkForPeersToMakeStable(...);

>+    return onlyUndemotes ? TypeConsensus_Undemotes : TypeConsensus_Bad;

It's not clear to me whether the boolean state of onlyUndemotes is being used correctly.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: easier to read, i hope

Took into account nits from previous comments. Requesting review again to make sure logic is clearer.

Comment 11

[Robert Sayre]

Comment on attachment 387269 [details] [diff] [review]
easier to read, i hope

>diff -r 5c4c1f31d00b js/src/jstracer.cpp

> 
>+JS_REQUIRES_STACK static inline void

JS_INLINE

>+enum TypeCheckResult
>+{
>+    TypeCheck_Okay,         /* Okay: same type */
>+    TypeCheck_Promote,      /* Okay: Type A needs f2i() */
>+    TypeCheck_Demote,       /* Okay: Type A needs i2f() */
>+    TypeCheck_Undemote,     /* Bad: Slot is undemotable */
>+    TypeCheck_Bad           /* Bad: incompatible types */
>+};

Seems like the result show be more obviously bad/good or true/false. Are there refactorings we could do to avoid 5 kinds of return types here? Follow up bugs are ok.

>+
>+    TypeConsensus
>+    checkTypes(TreeInfo* ti)
>+    {
>+        if (length() != ti->typeMap.length())
>+            return TypeConsensus_Bad;
>+
>+        bool has_undemotes = false;
>+        for (unsigned i = 0; i < length(); i++) {
>+            TypeCheckResult result = checkType(i, ti->typeMap[i]);
>+            if (result == TypeCheck_Bad)
>+                return TypeConsensus_Bad;
>+            if (result == TypeCheck_Undemote)
>+                has_undemotes = true;
>+            slots[i].lastCheck = result;
>+        }
>+        if (has_undemotes)
>+            return TypeConsensus_Undemotes;
>+        return TypeConsensus_Okay;
>+    }

Seems suspicious. Why not return has soon has you find that (result == TypeCheck_Undemote) ? Is it OK that only some slots[i].lastCheck fields are assigned if (result == TypeCheck_Bad) ?

>+  private:
>+    JS_REQUIRES_STACK JS_ALWAYS_INLINE void
>+    addSlot(jsval* vp)
>+    {
>+        bool promote = false;
>+        if (isNumber(*vp) &&
>+            ((useTracker && isPromoteInt(mRecorder.get(vp))) || (!useTracker && isInt32(*vp)))) {
>+            promote = true;
>+        }
>+        slots.add(SlotInfo(vp, promote));
>+    }

change this to

slots.add(SlotInfo(vp, checkPromoteSlot(vp));

and then you don't need the local bool variable.


>+JS_REQUIRES_STACK TypeConsensus
>+TraceRecorder::peerTypeStability(SlotMap& smap, VMFragment** pPeer)

looks ok, maybe rename to hasPeerTypeStability or something. No big deal.


>+JS_REQUIRES_STACK bool
>+TraceRecorder::closeLoop(SlotMap* slotMap, TypeConsensus& consensus)

I think this method is too long. If you agree, please break it up.

Comment 12

[Robert Sayre]

Oops, used some English5 there. Sorry.

> 
> Seems like the result show be more obviously bad/good or true/false. 

s/show/should

> 
> Seems suspicious. Why not return has soon has you find that

as soon as

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: new version

New version changes a few things that the recursion patch uses. useTracker got replaced by slotOffset.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> >+JS_REQUIRES_STACK static inline void
> 
> JS_INLINE

I'm not sure I see the reason to obfuscate |inline| behind a macro this way, and we don't do it many other places.  Why here?

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #11)
> JS_INLINE

Fixed - turns out this was old code, I had just moved it from point A to B.

> Seems like the result show be more obviously bad/good or true/false. Are there
> refactorings we could do to avoid 5 kinds of return types here? Follow up bugs
> are ok.

Yes, I could use flags instead and return true/false. Will do follow-up for that.

> Seems suspicious. Why not return has soon has you find that (result ==
> TypeCheck_Undemote) ? Is it OK that only some slots[i].lastCheck fields are
> assigned if (result == TypeCheck_Bad) ?

Yes that's okay. I've documented this a bit better now. The reason I don't return immediately on TypeCheck_Undemote is because I want to precisely mimic the old code, and leave changes in functionality for follow-up bugs.

The old code does it to try and mark all undemotes at once, so it doesn't have to discover successive ones by failing to compile over and over. But this could still be done without having the somewhat confusing return flow.

> slots.add(SlotInfo(vp, checkPromoteSlot(vp));

Fixed.

> looks ok, maybe rename to hasPeerTypeStability or something. No big deal.

Leaving without "has" for now since "has" feels boolean to me.

> I think this method is too long. If you agree, please break it up.

I shortened it a bit by hoisting some code out. I hope it's better now.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #14)
> I'm not sure I see the reason to obfuscate |inline| behind a macro this way,
> and we don't do it many other places.  Why here?

Agree IMO, inline keyword looks nicer.

Comment 17

[Robert Sayre]

(In reply to comment #16)
> (In reply to comment #14)
> > I'm not sure I see the reason to obfuscate |inline| behind a macro this way,
> > and we don't do it many other places.  Why here?
> 
> Agree IMO, inline keyword looks nicer.

http://mxr.mozilla.org/mozilla-central/source/js/src/jstypes.h#168

ah, it doesn't help much in our new __cplusplus world, does it? :) no JS_INLINE needed.

Comment 18

[Andreas Gal :gal]

Comment on attachment 387996 [details] [diff] [review]
new version

/*
 * Text
 */

Not

/* Text
 */

Comment 19

[Robert Sayre]

This patch seemed to cause test failures, and I backed it out.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: refreshed

I refreshed this patch, ran mochitests locally on OS X and Linux multiple times without problems. Also got back greens from the try server.

With someone's permission I'd like to try and land this again.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

seems to have stuck this time

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

Comment 22

[Robert Sayre]

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