530255 - On trace, replace GetFromClosure/GetClosureVar/GetClosureArg with specialized code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I ran into GetFromClosure being about 30% of some of the dromaeo JS tests today. Specifically, the js_GetPropertyHelper call.  Brendan muttered something about the property cache; is there a way we can make use of it here?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I tried actually doing this, by cribbing the do_getprop_with_obj code in the JSOP_GETPROP case in jsops.cpp.  It helps a little bit (maybe 5% overall improvement).  The time spent under js_GetPropertyHelper drops to almost nothing, but we get more js_NativeGet (which is where most of the js_GetPropertyHelper time was anyway; resolve was only 1/5 of the js_GetPropetyHelper time), and we get a bit of js_FullTestPropertyCache popping in.  The js_NativeGet is spending its time largely calling CallPropertyOp and in itself.

I dug around a bit, and what ends up in the propcache here, if I did things right, is an sprop whose getter is a non-stub.  Specifically, it's js_GetCallVar.  So even on cache hit we fall into the js_NativeGet case.  We save the resolve costs but pay the cache test costs; that's about it.

Looking through what CallPropertyOp and company are doing, I wonder whether it's worth special-casing the case when we find the sprop on the call object itself and are looking at js_GetCallVar.  We have all this info in the ClosureVarInfo, including the |id| and |slot| from when we traced it.  Are these at all related to the call object's reserved slots?  If so, can we just directly grab what we want from there instead of going through the packing/unpacking rigmarole that js_NativeGet forces us into (call it, so it can call the getter func, which then calls a func that handles all sorts of different gets and sets, which can then convert the id into a slot offset, etc, etc).

Does any of that make sense?

Comment 2

[Brendan Eich [:brendan]]

bz: yes, that all makes sense to me. Dmandelin should weigh in for sure.

Shape determines getter and setter. The only issue is whether the call object's frame is still active, in which case we need to get the value from fp->slots, or the frame has gone away, in which case we need to get the value from the call object's reserved slot.

Shape does not tell us which (frame active still, or gone away) is the case, unfortunately. Maybe it should...

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Shape does not tell us which (frame active still, or gone away) is the case,

That's a non-issue.  In GetFromClosure we do:

    JSStackFrame* fp = (JSStackFrame*) call->getPrivate();

and then only get into the codepath I'm touching if !fp.  So the frame has gone away.

Backing up a bit, the only way to get into GetFromClosure is via GetClosureArg and GetClosureVar.  And the only way to get into those is via TraceRecorder::callProp.  callProp is only called for Call objects and only if the property resolved on the object itself.

Now in callProp, we make sure to call GetClosureArg if sprop->getter == js_GetCallArg.  Otherwise we call GetClosureVar.  If the call object has an fp in callProp(), then we RETURN_STOP if the getter is not one of js_GetCallArg or js_GetCallVar.  If it has no fp by that point, we seem not to.  I think we should consider changing that, so that we can guarantee that sprop->getter is one of js_GetCallVar and js_GetCallArg in GetFromClosure; if we really need to handle the "null fp and not arg or var" case, then we should use a separate function for that, I think.

If we assume that our setters are guaranteed, then the js_GetPropertyHelper case currently does the following, effectively:

  js_NativeGet(cx, obj, obj2, sprop, getHow, vp) 

where obj == obj2 in our case, and getHow is JSGET_METHOD_BARRIER.

js_NativeGet leaves trace if we're the global (we're not), and then, since JSGET_METHOD_BARRIER is set and because we have a non-stub getter ends up rooting some stuff and calling sprop->get.  Does some isMethod() stuff (relevant here?  Comments in callProp claim not yet pending bug 514046) and then calls the getter, like so:

  getter(cx, obj, SPROP_USERID(this), vp)

SPROP_USERID is the shortid or the id, depending, as a jsval.  callProp assumes that sprop has a shortid, and perhaps should assert SPROP_HAS_SHORTID.  So we have a shortid here too, and that's what gets passed to the getter.

Now js_GetCallVar does:

  CallPropertyOp(cx, obj, id, vp, JSCPK_VAR, JS_FALSE)

and js_GetCallArg does:

  CallPropertyOp(cx, obj, id, vp, JSCPK_ARG, JS_FALSE)

CallPropertyOp checks the class (we already know it's js_CallClass), does something in case JSCPK_ARGUMENTS (which neither of these is), asserts that our id (after JSVAL_TO_INT) is < nargs or n.i.nvars, depending on whether we're in the ARG or VAR case, and then if !fp adds CALL_CLASS_FIXED_RESERVED_SLOTS to the id, adds nargs in the VAR case and does JS_GetReservedSlot on the result.

JS_GetReservedSlot does some lock/unlock fun, then sanity-checking on the slot index and then does STOBJ_GET_SLOT(obj, JSSLOT_START(clasp) + index).

Comment 4

[David Mandelin [:dmandelin]]

bz, I think your idea can be concisely explained as "on trace, replace GetFromClosure/GetClosureVar/GetClosureArg with specialized code", which I think is a great idea and could probably be used in other places to help both on and off trace. I briefly scanned comment 3 and it looks basically right. It would be interesting to think about how much specialization we can do in a specialized builtin vs. LIR, and decide which is better. (But the builtin seems easier and is a logical first step.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

This seems to work fine; speeds up the slice test on dromaeo from 110run/s to 170runs/s or so.  Open questions:

1)  Am I correct in assuming that there will in fact be slots corresponding to
    nargs/nvars respectively?  Or do I actually need to go through the check
    against nslots that JS_GetReservedSlot does in this case as well?
2)  Is it ok that I'm skipping the locking JS_GetReservedSlot does?
3)  Should I just assume (and JS_Assert, perhaps?) that the slots are in
    dslots?  Eliminates a branch.

I think it would be pretty simple to do this !fp case entirely in LIR even in the STOBJ_GET_SLOT formulation; if we assume the slot's in dslots, it would be even simpler.  The hardest part is likely getting that js_GetCallObjectFunction bit into LIR.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and I guess another question:

4)  Is it ok that I'm assuming the slot set stays immutable, effectively?  Seems
    to me like that should be the case, so shape really doesn't matter, nor does
    propcache, right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I filed bug 532477 about similar treatment for the set case.

I do wonder about one asymmetry here.  For get, we use the fp for stuff even if it's not reachable. For set we do not.  Why not?

Comment 8

[David Mandelin [:dmandelin]]

(In reply to comment #5)
> Created an attachment (id=415690) [details]
> Possible patch

That was quick. Nice speedup. I don't know the answer to questions 1 and 2.

The patch looks good to me, except for 2 things:

    1. I don't think adj_slot can be modified that way--it is already being
       used for something else (see the early exit just above the main part
       you modified in GetFrom Closure). Specifically, it maps a "slot" number
       in (argv or slots) to a native stack frame index (relative to the start
       of that native frame). 

    2. I think your patch makes ClosureVarInfo::id obsolete, so it should be
       removed.

> 3)  Should I just assume (and JS_Assert, perhaps?) that the slots are in
>     dslots?  Eliminates a branch.

Sounds like a good idea to me, *if* it speeds things up even more. 
 
> 4)  Is it ok that I'm assuming the slot set stays immutable, effectively? 
> Seems to me like that should be the case, so shape really doesn't matter, nor
> does propcache, right?

I believe so. The slot should not change unless the local scope gets modified by eval or something like that, and I think we put in shape guards to protect against that (in traverseScopeChain).

> I think it would be pretty simple to do this !fp case entirely in LIR even in
> the STOBJ_GET_SLOT formulation; if we assume the slot's in dslots, it would be
> even simpler.  The hardest part is likely getting that js_GetCallObjectFunction
> bit into LIR.

That bit is just in an assertion, it seems, so maybe it's not that bad.

> I filed bug 532477 about similar treatment for the set case.
>
> I do wonder about one asymmetry here.  For get, we use the fp for stuff 
> even if it's not reachable. For set we do not.  Why not?

I don't remember the exact problem for the case you are talking about. I was thinking it might be the 'weak update' problem, but I think that might be for a slightly different situation. I think the problem might be that if the fp is not reachable, that outer var might later be part of an outer trace that calls the current trace. And so we'd need to check that the type is the same, or otherwise handle the problem that our setting the variable might mess up the types of the outer trace.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #5)
> 1)  Am I correct in assuming that there will in fact be slots corresponding to
>     nargs/nvars respectively?  Or do I actually need to go through the check
>     against nslots that JS_GetReservedSlot does in this case as well?

All reserved slots are allocated early -- see js_GetCallObject:

    callobj = js_NewObjectWithGivenProto(cx, &js_CallClass, NULL, fp->scopeChain);
    if (!callobj ||
        !js_EnsureReservedSlots(cx, callobj, fp->fun->countArgsAndVars())) {
        return NULL;
    }

but note these slots are not updated until fp goes away.

How do we know fp has gone away without checking the private slot? Would it help if the Call object's shape changed when it lots its frame?

> 2)  Is it ok that I'm skipping the locking JS_GetReservedSlot does?

Yes! No cross-thread sharing of Call objects, you die if you try. Screw 'em if they can't take a joke.

> 3)  Should I just assume (and JS_Assert, perhaps?) that the slots are in
>     dslots?  Eliminates a branch.

Please read the code in jsfun.cpp, specifically the definitions used by this code in js_PutCallObject:


    /*
     * Since for a call object all fixed slots happen to be taken, we can copy
     * arguments and variables straight into JSObject.dslots.
     */
    JS_STATIC_ASSERT(JS_INITIAL_NSLOTS - JSSLOT_PRIVATE ==
                     1 + CALL_CLASS_FIXED_RESERVED_SLOTS);

> I think it would be pretty simple to do this !fp case entirely in LIR even in
> the STOBJ_GET_SLOT formulation; if we assume the slot's in dslots, it would be
> even simpler.  The hardest part is likely getting that js_GetCallObjectFunction
> bit into LIR.

That's all quite doable. Go for it!

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #8)
> I don't think adj_slot can be modified that way

I didn't modify it; I just overloaded it.  The old adj_slot takes a JSStackFrame* and adjusts slots for the frame.  The new one takes a JSObject* and adjusts slots for the object.  This should be fine, I would think, unless my C++ compiler is very broken.  ;)

> I think your patch makes ClosureVarInfo::id obsolete

Er, yes.  I meant to remove and forgot.

> Sounds like a good idea to me, *if* it speeds things up even more. 

Looks like a 5% or so win over here.  Takes dromaeo string tests from 460 to 480.  Not sure what it would do to the overall dromaeo number; this closure stuff is more of a bottleneck on the faster tests...

> and I think we put in shape guards to protect against that

Excellent.

> That bit is just in an assertion, it seems, so maybe it's not that bad.

It's not just an assertion; the VAR (not ARG) version of the slot-adjustment code needs to know the nargs for the function.  But more on this below.

> otherwise handle the problem that our setting the variable might mess up the
> types of the outer trace.

Which is why the outer trace needs to check on get?  OK, makes sense.

(In reply to comment #9)
> How do we know fp has gone away without checking the private slot?

We don't.  We check it when tracing, in callProp and setCallProp.

At least the latter seems to assume that fp won't go away for the lifetime of the trace, if it's there when setCallProp is called.  Is that a sane assumption?

> Please read the code in jsfun.cpp

Yes, I did in fact read it.  Hence my asking about whether we're ok with assuming the same here!  Sounds like you're ok with it, at least.

> That's all quite doable. Go for it!

I think I will.  Will Call objects for different functions have different shapes?  That is, can I assume that js_GetCallObjectFunction returns the same JSFunction if the shape is the same?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #9)
> > How do we know fp has gone away without checking the private slot?
> 
> We don't.  We check it when tracing, in callProp and setCallProp.
> 
> At least the latter seems to assume that fp won't go away for the lifetime of
> the trace, if it's there when setCallProp is called.  Is that a sane
> assumption?

This is a q for dmandelin.

> > Please read the code in jsfun.cpp
> 
> Yes, I did in fact read it.  Hence my asking about whether we're ok with
> assuming the same here!  Sounds like you're ok with it, at least.

Static assertions have your back ;-).

> > That's all quite doable. Go for it!
> 
> I think I will.  Will Call objects for different functions have different
> shapes?  That is, can I assume that js_GetCallObjectFunction returns the same
> JSFunction if the shape is the same?

No, that's what I'm getting at. Shapes do not cover private slot or reserved *value* differences. Perhaps they could for Call objects to speed up the JITted code, provided they evolve predictably.

Plain objects with methods (function valued properties, either set from a null closure lambda via assignment or property init for class Object instances, or on first call for other kinds of function values) include method values in their shapes to speed up method calls. You might be able to do the same for the callee slot of Call objects.

Do you see N > 1 Call objects for a given callee function object in the code you're benchmarking? I.e. repeated activations of the same heavyweight?

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Er, yes.  I meant to remove and forgot.

Also, resolveFlags is unused, and callDepth is only used in debug code...  Making those changes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Do you see N > 1 Call objects for a given callee function object

It's really hard to tell because we have such multiple activations all over our chrome (even in the simplified layout-debugger chrome), as does dromaeo; I'm not sure whether they happen as part of the timed stuff, though.

But ok.  I have the answer I needed.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Now in callProp, we make sure to call GetClosureArg if sprop->getter ==
> js_GetCallArg.  Otherwise we call GetClosureVar.  If the call object has an fp
> in callProp(), then we RETURN_STOP if the getter is not one of js_GetCallArg or
> js_GetCallVar.  If it has no fp by that point, we seem not to.  I think we
> should consider changing that, so that we can guarantee that sprop->getter is
> one of js_GetCallVar and js_GetCallArg in GetFromClosure; if we really need to
> handle the "null fp and not arg or var" case, then we should use a separate
> function for that, I think.

I missed this, it's a good point. The Call object is not addressable as an object in the language, except due to severe bugs (two-arg eval, blech) that we've fixed when they arise (none known now; debuggers better not expose Call objects!).

So the set of getters and setters for properties defined on it is bounded by the VM. I think it's just this set

getters: GetCallArguments js_GetCallArg js_GetCallVar js_GetCallVarChecked
setters: SetCallArguments SetCallArg SetCallVar

Igor: any others?

> js_NativeGet leaves trace if we're the global (we're not), and then, since
> JSGET_METHOD_BARRIER is set and because we have a non-stub getter ends up
> rooting some stuff and calling sprop->get.  Does some isMethod() stuff
> (relevant here?  Comments in callProp claim not yet pending bug 514046)

Not relevant.

Boris, this is great -- optimize the hell out of this path!

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

> js_GetCallVarChecked

Aha!  That might explain why we weren't testing for js_GetCallVar.  How should js_GetCallVarChecked differ?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm.  It differs by checking for escaping closures.  What should this look like on trace?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Hmm.  It differs by checking for escaping closures.  What should this look like
> on trace?

This is to cope with debuggers, which should not be active if the JIT is running.

/be

Comment 18

[David Mandelin [:dmandelin]]

> > I don't think adj_slot can be modified that way
>
> I didn't modify it; I just overloaded it.  The old adj_slot takes a
> JSStackFrame* and adjusts slots for the frame.  The new one takes a JSObject*
> and adjusts slots for the object.  This should be fine, I would think, unless
> my C++ compiler is very broken.  ;)

I missed that. I still think overloading that name is confusing, though. (I also don't like my original name choice of adj_slot, and now that there are more functions in play, it's even worse.) Hmmm...

                            input                   output
   'old' adj_slot    frame argv/slot index    native frame index
   'new' adj_slot    frame argv/slot index    call object slots index

Could the old one be named something like 'nativeSlotFromInterpSlot' or 'interpSlotToNativeSlot', and the new 'interpSlotToCallObjSlot'? I used short names at first because I like looking at short names, but the concepts involved here seem subtle and worth more expressive names.

> > How do we know fp has gone away without checking the private slot?
> 
> We don't.  We check it when tracing, in callProp and setCallProp.
> 
> At least the latter seems to assume that fp won't go away for the lifetime of
> the trace, if it's there when setCallProp is called.  Is that a sane
> assumption?

I'm not sure exactly what these questions mean, but thinking about them made me realize there is a bug in the current code. I will file it and link it to the other bug.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I'm not sure exactly what these questions mean

The basic question is whether it's possible to record with non-null fp and then later end up in this code with null fp when executing the trace.  Or vice versa...

As for adj_slot, it's gone locally in favor of a slot() function, and even that's about to go away in favor of some LIR, I hope.  At least if I can get all the crashes sorted out.  ;)

Comment 20

[David Mandelin [:dmandelin]]

(In reply to comment #19)
> > I'm not sure exactly what these questions mean
> 
> The basic question is whether it's possible to record with non-null fp and then
> later end up in this code with null fp when executing the trace.  Or vice
> versa...

I got that part, but I don't know which 'fp' you are talking about when you say 'non-null fp'. I guess you are probably talking about callobj->getPrivate(), in which case I think:

  - If the frame was in range, then the outer function will necessarily be active on any future trace call. Then:

    - If the frame was the trace entry frame, then on any future trace call that frame should exist as a JSStackFrame* and be stored in callobj->getPrivate().

    - If the frame was not the trace entry frame (i.e., is on trace at least one call deep), then I think that JSStackFrame* will not exist, and the callobj also will not exist.

  - If the frame was not in range, then the frame was not part of the trace, so pretty much anything could happen: the private could be null, or non-null, or the trace could be called from another trace that has that frame as part of its native stack area.

> As for adj_slot, it's gone locally in favor of a slot() function, and even
> that's about to go away in favor of some LIR, I hope.  At least if I can get
> all the crashes sorted out.  ;)

Sounds good.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I guess you are probably talking about callobj->getPrivate()

Yes, I am.  Sorry about not making that clear.

So it sounds like if we have a non-null callobj->getPrivate() and it's not in range, then there's no guarantee what will happen and we have to have the trace handle both having a private and not, right?  On the other hand, if the private is in range, then we know we'll have such a private in the future (at least if we hit this code at all).

What about if callobj->getPrivate() is null while recording?  Can it become non-null when executing the trace?

I have the LIR working for bug 532477, but given my questions above and bug 532568 I'm not quite sure what the best way to proceed is here....

Comment 22

[Boris Zbarsky [:bzbarsky]]

I had a nice long conversation with dmandelin about this stuff.  The highlight conclusions:

1)  If at record time the call object's private is non-null and in-reach, then
    it will continue to be non-null on trace (assuming there's a call object at
    all).
2)  If at record time the call object's private is null, then it will also be
    null on trace.  This is guaranteed by the parent guard in guardCallee.
3)  If at record time the call object's private is non-null and not in-reach,
    then at run time it might be null or not, etc.  This is the hard case.
4)  The nargs of the call object's JSFunction will be the same on trace as
    during record time, thanks to the private guard in guardCallee.

Plan of action:

* Item 1 above is handled nicely already.  Leave that as-is. 
* Item 2 above can be done in hand-rolled LIR reading dslots, and will be.
* Item 3 above needs to keep using the builtin we use now, but the case when
  the private is null in that builtin can be optimized as in "Proposed patch" 
* Item 4 above just means that the LIR can be nice and simple, since nargs can
  be baked in at trace time.
* For bug 532477, similar.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch per that plan of action

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Please check the callProp changes over really carefully.  I assumed we needed the traverseScopeChain even if getPrivate() is null, and that we still need the nr.* stuff at the end, but that we could skip the getCoercedType guard because unbox_jsval would do its own guarding.  That all could use double-checking.  Not sure whether this is easier to read, or the version without -w...  Pick one or both.

Comment 25

[David Mandelin [:dmandelin]]

(In reply to comment #24)
> Created an attachment (id=415806) [details]
> Same as diff -w
> 
> Please check the callProp changes over really carefully.  I assumed we needed
> the traverseScopeChain even if getPrivate() is null,

traverseScopeChain gets us to the right JSObject* callobj, so if the call object is to be used at all, it is necessary. Also, it adds the needed shape guards to make sure no shadowing happened in the interpreter due to eval. So I believe it is necessary in all cases (except when we read directly from trace stack area).

> and that we still need the nr.* stuff at the end, 

The nr.* stuff is used by some callers to figure out how access the value for further operations (e.g., increment ops), so I'm pretty sure we still need that.

> but that we could skip the getCoercedType guard because
> unbox_jsval would do its own guarding.  

Yes.

Comment 26

[David Mandelin [:dmandelin]]

Comment on attachment 415806 [details] [diff] [review]
Same as diff -w

I assume you tested it with my ad hoc closures suite. Firing up gmail and Google Docs for to see if the toolbars appear and things basically work seems to be another good test for closure-related stuff.

The patch looks great. A comment on one of the traits classes to explain the meaning of adj_slots and/or slot_offset might be nice, if we think it would stay valid.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I assume you tested it with my ad hoc closures suite.

Yes.

> Firing up gmail and Google Docs

Tested just now.  Looks good so far.

> A comment on one of the traits classes to explain the
> meaning of adj_slots and/or slot_offset might be nice

Added:

    // Adjust our slot to point to the correct slot on the native stack
    static inline uint32 adj_slot(JSStackFrame* fp, uint32 slot) { return fp->argc + slot; }
    // Get the right frame slots to use our slot index with.
    static inline jsval* slots(JSStackFrame* fp) { return fp->argv; }
    // Get the right object slots to use our slot index with.
    static inline jsval* slots(JSObject* obj) {
        // We know Call objects use dslots.
        return obj->dslots + slot_offset(obj);
    }
    // Get the offset of our object slots from the object's dslots pointer.
    static inline uint32 slot_offset(JSObject* obj) {
        return JSSLOT_START(&js_CallClass) +
            CALL_CLASS_FIXED_RESERVED_SLOTS - JS_INITIAL_NSLOTS;
    }
    // Get the maximum slot index of this type that should be allowed
    static inline uint16 maxObjSlots(JSObject* obj) {
        return js_GetCallObjectFunction(obj)->nargs;
    }

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 415806 [details] [diff] [review]
Same as diff -w

>+        /*
>+         * Get the value from the object.  We know we have a Call

Ok, nit about *wanting* French spacing -- it's the latest fashion ;-).

>+         * object, and that our slot index is fine, so don't monkey
>+         * around with calling the property getter (which just looks
>+         * in the slot) or calling js_GetReservedSlot.  Just get the
>+         * slot directly.

Could cite static assertions in jsfun.cpp here to help people connect the dots.

> struct ArgClosureTraits
> {
>     static inline uint32 adj_slot(JSStackFrame* fp, uint32 slot) { return fp->argc + slot; }
>     static inline jsval* slots(JSStackFrame* fp) { return fp->argv; }
>+    static inline jsval* slots(JSObject* obj) {
>+        // We know Call objects use dslots.
>+        return obj->dslots + slot_offset(obj);
>+    }
>+    static inline uint32 slot_offset(JSObject* obj) {
>+        return JSSLOT_START(&js_CallClass) +
>+            CALL_CLASS_FIXED_RESERVED_SLOTS - JS_INITIAL_NSLOTS;
>+    }
>+    static inline uint16 maxObjSlots(JSObject* obj) {

This method's name is camelCaps unlike the rest. Suggest sticking to a style within a class or struct, even if it is inconsistent with overall style (TraceRecorder is a mess but let's not spread its inconsistency if we can help it). How about slot_count as the name instead of maxObjSlots?

>+        if (sprop->getter == js_GetCallArg)
>+            offset += ArgClosureTraits::slot_offset(obj);
>+        else if (sprop->getter == js_GetCallVar ||
>+                 sprop->getter == js_GetCallVarChecked)
>+            offset += VarClosureTraits::slot_offset(obj);
>+        else
>+            RETURN_STOP("dynamic property of Call object");

Nit: this if-else chain wants bracing all around because of the multiline condition of the second if.

>     if (sprop->getter == js_GetCallArg)
>         ci = &GetClosureArg_ci;
>-    else
>+        else if (sprop->getter == js_GetCallVar ||
>+                 sprop->getter == js_GetCallVarChecked)
>         ci = &GetClosureVar_ci;
>+        else
>+            RETURN_STOP("dynamic property of Call object");

Ditto.

>@@ -12053,16 +12103,24 @@ TraceRecorder::guardCallee(jsval& callee
>     LIns* callee_ins = get(&callee);
> 
>     treeInfo->gcthings.addUnique(callee);
>     guard(true,
>           lir->ins2(LIR_peq,
>                     stobj_get_private(callee_ins),
>                     INS_CONSTPTR(callee_obj->getPrivate())),
>           branchExit);

Blank line here.

>+    /*
>+     * As long as we have this parent guard, we're guaranteed that if we record
>+     * with a Call object which has a null getPrivate(), then on trace that
>+     * Call object will continue to have a null private.

Still think (per my other review) this should be revised to say why. As is it is kinda circular.

>  callProp and
>+     * setCallProp depend on this and document where; if this guard is removed
>+     * make sure to fix those methods.  Search for the "parent guard" comments
>+     * in them.
>+     */
>     guard(true,
>           lir->ins2(LIR_peq,
>                     stobj_get_parent(callee_ins),
>                     INS_CONSTOBJ(OBJ_GET_PARENT(cx, callee_obj))),
>           branchExit);
>     return RECORD_CONTINUE;
> }
> 

Great patch, keep 'em coming!

/be

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Ok, nit about *wanting* French spacing -- it's the latest fashion ;-).
> Could cite static assertions in jsfun.cpp here to help people connect the dots.

Done.

> This method's name is camelCaps unlike the rest.

Fixed to slot_count.

> this if-else chain wants bracing all around
> Ditto.
> Blank line here.

Done.

> Still think (per my other review) this should be revised to say why. 

Done (same comment as in other bug).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 31

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/17bd8551d437

Comment 32

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/17bd8551d437