469237 - TM: "Assertion failure: OBJ_IS_CLONED_BLOCK(obj)" with "with"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> for (let a=0;a<3;++a) for (let b=0;b<3;++b) if ((g=a|(a%b))) with({}){}
Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at ../jsobj.cpp:2067

This testcase only triggers the assertion failure when it runs at the top level (not in a function), and 'g' seems to have to be undeclared :(

Only happens with -j.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

$ ./js-dbg-tm-intelmac -j
js> for (let a=0;a<3;++a) for (let b=0;b<3;++b) if ((g=a|(a%b))) with({}){}
Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at ../jsinterp.cpp:7160

-j only too.

Duping.

Comment 2

[Brendan Eich [:brendan]]

Not a dup of bug 472528, which is dependent on upvar (has a lambda, no with). This bug is about with.

/be

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

To clarify, the testcase in comment #0 currently asserts with OBJ_IS_CLONED_BLOCK assertion and not the one listed in comment #1.

Comment 4

[Jim Blandy :jimb]

The block in question is the outermost block, holding a.

The OBJ_IS_CLONED_BLOCK test really asserts that the block doesn't own its own map; the block in question is indeed a clone of the compiler-allocated block object, as it should be.

But why does this block own its own map?  For some reason, the assignment to 'g' in the 'if' is adding the property to the outermost let block, not to the global object.  I need to understand what's going on with the property cache probe code in the JSOP_SETNAME code that misidentifies where the new property should go.

Comment 5

[Brendan Eich [:brendan]]

The bug is that TraceRecorder::record_JSOP_BINDNAME does not call js_GetScopeChain, it uses cx->fp->scopeChain directly. It looks like other ops (perhaps not all that use cx->fp->scopeChain, though) suffer the same bug.

/be

Comment 6

[Jim Blandy :jimb]

If record_JSOP_BINDNAME did call js_GetScopeChain, that would clone blocks, which isn't necessary here.  We do need to ensure there's a call object, though; I wonder where that gets taken care of.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> If record_JSOP_BINDNAME did call js_GetScopeChain, that would clone blocks,
> which isn't necessary here.  We do need to ensure there's a call object,
> though; I wonder where that gets taken care of.

Search for JSFUN_HEAVYWEIGHT_TEST in jsinterp.cpp.

/be

Comment 8

[Robert Sayre]

ping? no activity for 10 days...

Comment 9

[Brendan Eich [:brendan]]

I can answer questions if any -- mrbkap too I bet.

/be

Comment 10

[Jim Blandy :jimb]

Summary, written up for a different audience; please forgive restatement:
----
In the following code:

for (let a=0;a<3;++a)
  for (let b=0;b<3;++b)
    if ((g=a|(a%b)))
      with({})
        {}

SpiderMonkey with the JIT enabled ends up creating 'g' as a
property on the lexical block object for 'let a', instead of
on the global object where it belongs.

The assignment to 'g' is handled with two bytecodes: a
JSOP_BINDNAME chooses which object 'g' should be a property of
(the global object, in this case), and pushes that object on
the stack.  Then, a JSOP_SETNAME stores the value in that
object's property 'g'.

This test case arranges for the following sequence of events:

First, the 'if' condition is false the first four times it's
reached, so we JIT the path where the 'then' is not taken.
This path does include the assignment to 'g'.

Second, we eventually do reach the 'with' statement.  The JIT
refuses to touch 'with' statements, so we never JIT this
branch.  Another quirk of 'with' statements is that they're
not optimized: whereas the interpreter normally doesn't clone
blocks until they're closed over, entering a 'with' forces the
interpreter to go ahead and bring fp->scopeChain fully up to
date: a 'with' object at the top; then two cloned blocks for
the 'for (let' loops; and then the global object.

Third, we re-enter the JITted code, but take a side exit
associated with the '%' operator (right operand zero), and
fall back out to the interpreter.  This side exit is at a
point between the JSOP_BINDNAME and the JSOP_SETNAME, so the
former is done by JITted code, whereas the latter is done by
the interpreter.

As it turns out, the effect of the 'with' on fp->scopeChain
means that the JITted code does JSOP_BINDNAME's job
incorrectly, selecting a's block as the target of the
JSOP_SETNAME.  That block now owns its own scope, which
properly cloned blocks never should, and we assert.

At the moment, it seems to me that record_JSOP_BINDNAME is
confused.  It seems to assume that fp->scopeChain has some
fixed relationship to the object in the scope chain that holds
the name being bound --- but that isn't true at all: as the
test case shows, an arbitrary condition can decide whether we
evaluate a 'with' block, which may or may not add new entries
to the head of fp->scopeChain.  (For what it's worth, similar
conditions held before 480132 landed, too.)

Doing JSOP_BINDNAME properly on trace entails some
interactions with the property cache that I don't yet
understand, to get TraceRecorder::test_property_cache to
produce the right guards.  So the time has come for me to
learn how the property cache works; that's what I'm doping my
way through now.

Comment 11

[Jim Blandy :jimb]

IRC conversation: if we don't abort, then the object BINDNAME identifies as the appropriate home for the property will always be correct: since we can't trace code that closes over anything other than the global object, the global is the only home BINDNAME will ever find.

So record_JSOP_BINDNAME might as well simply embed the address of that object in the trace as a constant.

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate correct code for JSOP_BINDNAME.

Here's a patch I'm testing now, posted to show the kind of fix I have in mind.  Does this look plausible?  It fixes the test case, and allows code like the following to be traced:

t=0;
let (x=0) {
  function g() {};
  for (i=0; i<10; i++) {
    t = t*2+1;
  };
};
print (t);

Comment 13

[Jim Blandy :jimb]

As Brendan mentions, it seems like there are other, analogous cases in jstracer.cpp.  Looking into that.

Comment 14

[Jim Blandy :jimb]

Patch passes js/tests and js/src/trace-test.js.

Comment 15

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate correct code for JSOP_BINDNAME.

We can only trace BINDNAME when it finds a variable on the global
object.  Since we already guard on the global object and its shape, we
might as well simply embed the global object itself into the trace.

It is incorrect for BINDNAME to generate code to fetch fp->scopeChain,
since that could have block objects added to it off-trace.  By the
same token, SETNAME should not insist that scopeChain be unchanged ---
only that the actual object whose property is being set is the global
object.

Comment 16

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate correct code for JSOP_BINDNAME.

Revisions: Call js_FindProperty, not js_FindPropertyHelper.  Don't forget to drop the property.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 370714 [details] [diff] [review]
Bug 469237: Generate correct code for JSOP_BINDNAME.

>+    JSAtom* atom = atoms[GET_INDEX(cx->fp->regs->pc)];
>+    JSObject *obj, *pobj;
>+    JSProperty *prop;

Suggest blank line here for readability.

>+    if (! js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &pobj, &prop))
>+        ABORT_TRACE("JSOP_BINDNAME encountered an error looking up property");
>+    if (! prop)
>+        ABORT_TRACE("JSOP_BINDNAME couldn't find an existing binding");
>+    OBJ_DROP_PROPERTY(cx, pobj, prop);

House style wants a blank line before major (one or more line) comments unless previous char was {.

>+    /* We only trace functions closed over only the top-level environment.  */

Uber-nit: s/only trace/trace only/

>-    if (obj != cx->fp->scopeChain || obj != globalObj)
>+    if (obj != globalObj)
>         ABORT_TRACE("JSOP_SETNAME left operand is not the global object");
> 
>     // The rest of the work is in record_SetPropHit.

Great to see that old code gone. It never looked right, but we were concerned about cross-window calls. Or something, it was a while ago.

/be

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 370714 [details] [diff] [review]
Bug 469237: Generate correct code for JSOP_BINDNAME.

I'm worried that this patch could introduce a bug in some crazy case.  It seems like a more conservative fix would keep the call to test_property_cache and just change

>-    stack(0, obj_ins);
>+    stack(0, globalObj_ins);

(Please file a follow-up bug to make globalObj_ins equivalent to what you wrote here -- currently it hits memory, which is silly now that traces are per-global.)

Comment 19

[Jason Orendorff [:jorendorff]]

House style also has no space after the ! operator.

Comment 20

[Brendan Eich [:brendan]]

globalObj_ins is gone -- patch must be out of date.

/be

Comment 21

[Jim Blandy :jimb]

(In reply to comment #19)
> House style also has no space after the ! operator.

This is the hardest habit to break...

Comment 22

[Jim Blandy :jimb]

(In reply to comment #18)
> (From update of attachment 370714 [details] [diff] [review])
> I'm worried that this patch could introduce a bug in some crazy case.  It seems
> like a more conservative fix would keep the call to test_property_cache and
> just change
> 
> >-    stack(0, obj_ins);
> >+    stack(0, globalObj_ins);

Really?  In the absence of 'with', a given variable reference always refers to the same binding.  Once we've found it with js_FindProperty (which calls js_FindPropertyHelper, just as the interpreted JSOP_BINDNAME's call to js_FindIdentifierBase would), and verified that the variable is on the global object, then all that remains is to check the global object's shape --- which we always do before we enter the trace.

Or, to think of it another way --- what ins do you pass as obj_ins to test_property_cache?  One produced by TraceRecorder::scopeChain()?  That's definitely wrong, as fp->scopeChain varies unpredictably.  Or an ins computing the address of the global object?  Then test_property_cache is just going to check the global object's shape, which is redundant.  (Or maybe do nothing; I can't really tell.)

Comment 23

[Brendan Eich [:brendan]]

Quick comments:

Global shape doesn't need to be tested, so if the property is directly in the global object, test_property_cache doesn't do anything -- but it does fill the cache if necessary.

Good point about scopeChain(). Better to assert that we handle only the specific global case than to call scopeChain() to emit LIR that loads cx->fp->scopeChain.
 
/be

Comment 24

[Jim Blandy :jimb]

Looking into other uses of TraceRecorder::scopeChain.  Looks like TraceRecorder::name might be emitting guards that fail spuriously.

Comment 25

[Jason Orendorff [:jorendorff]]

(In reply to comment #22)
> In the absence of 'with', a given variable reference always refers to
> the same binding.  Once we've found it with js_FindProperty (which calls
> js_FindPropertyHelper, just as the interpreted JSOP_BINDNAME's call to
> js_FindIdentifierBase would), and verified that the variable is on the global
> object, then all that remains is to check the global object's shape --- which
> we always do before we enter the trace.

I'm convinced!  A brief comment to the effect of the first sentence above would be welcome, as a reminder of exactly what invariant we're depending on here.  Explicitly mentioning `with` is good.

This patch could result in a lookup being performed twice while recording; in the case of a resolve hook on the global object, it could be called twice on a miss.  I don't think we care, just noting the JSAPI-visible change.

(In reply to comment #20)
> globalObj_ins is gone -- patch must be out of date.

D'oh, I was looking at an old repo.

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 370714 [details] [diff] [review]
Bug 469237: Generate correct code for JSOP_BINDNAME.

>     if (obj != globalObj)
>+        ABORT_TRACE("JSOP_BINDNAME found a non-global variable");

How can this happen?

r=me but feel free to add a line or two of comments.

Comment 27

[Igor Bukanov]

(In reply to comment #22)
> In the absence of 'with', a given variable reference always refers to
> the same binding.

This is not true. Exactly the same behavior as with the "with" statement one can observe when a scope chain has more then one native scopes. For example, when an event handler is executed, its scope chain includes, besides the global object, also the element that was the source of the event. So in the following example the alert gives 3 1 as JSOP_BINDNAME implementing the assignment stores the reference to the element scope. 

<html>
<body>

<div id="myid" title="text" onclick="a=((delete a), 3); alert(a + ' ' + window.a)">
CLICK ME
</div>

<script>
a = 1;
document.getElementById('myid').a = 2;
</script>

</body>
</html>

Comment 28

[Igor Bukanov]

(In reply to comment #26)
> (From update of attachment 370714 [details] [diff] [review])
> >     if (obj != globalObj)
> >+        ABORT_TRACE("JSOP_BINDNAME found a non-global variable");
> 
> How can this happen?]

See the comment 27 for an example.

Comment 29

[Jim Blandy :jimb]

(In reply to comment #27)
> This is not true. Exactly the same behavior as with the "with" statement one
> can observe when a scope chain has more then one native scopes.

Thanks very much for explaining that!  The patch checks that the variable is found on the global object when recording, but it doesn't verify that there is nothing else on the scope chain whose properties might change.

Jason's intuitive paranoia FTW! :)

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> (In reply to comment #27)
> > This is not true. Exactly the same behavior as with the "with" statement one
> > can observe when a scope chain has more then one native scopes.
> 
> Thanks very much for explaining that!  The patch checks that the variable is
> found on the global object when recording, but it doesn't verify that there is
> nothing else on the scope chain whose properties might change.

That is what test_property_cache is for, ack! Which does need an obj_ins arg, presumably from scopeChain... but I don't want to go in circles here. Hence my suggestion to handle only the global object singleton scope chain case. Or is it clear how to deal with changing scope chain due to closure block cloning?

> Jason's intuitive paranoia FTW! :)

It's his mutant gift.

/be

Comment 31

[Jim Blandy :jimb]

Attachment: [committed] Bug 469237: Use offsetof instead of a magic constant.

This seems obvious, but...

Comment 32

[Jim Blandy :jimb]

(In reply to comment #30)
> That is what test_property_cache is for, ack! Which does need an obj_ins arg,
> presumably from scopeChain... but I don't want to go in circles here. Hence my
> suggestion to handle only the global object singleton scope chain case. Or is
> it clear how to deal with changing scope chain due to closure block cloning?

For the record: on IRC we discussed having a function returning an ins whose value was callee.__parent__.  We never push elements onto the scope chain that would have bindings for identifiers for which the compiler uses the NAME opcodes (e.g., BINDNAME): the compiler uses stack slot positions to refer to 'let' variables, and we punt on 'with'.  So starting searches at callee.__parent__ will never cause us to miss a binding.  And, callee.__parent__ is a trace constant, so test_property_cache can burn its expected shape into the trace, etc. with impunity.

(As opposed to scopeChain(), which may change, and thus can be burned into the trace only with punity.)

Comment 33

[Jim Blandy :jimb]

Got a new patch in testing; with luck, will be ready for review tomorrow morning.

Learned lots about the property cache (bug 487039).

Comment 34

[Jason Orendorff [:jorendorff]]

Comment on attachment 370986 [details] [diff] [review]
[committed] Bug 469237: Use offsetof instead of a magic constant.

Yes please!

Comment 35

[Jim Blandy :jimb]

(In reply to comment #34)
> (From update of attachment 370986 [details] [diff] [review])
> Yes please!
http://hg.mozilla.org/tracemonkey/rev/158f18c59bd9

Comment 36

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate correct code for JSOP_BINDNAME.

This patch regresses js1_5/Regress/regress-465132.js; posting for interested parties.

It is incorrect for BINDNAME to generate code to fetch fp->scopeChain,
since that could have block objects added to it off-trace.  Use a new
function, enclosingScopeChain, that does the right thing.

SETNAME need not abort recording if scopeChain has changed; it will
under normal circumstances.  Require only that the actual object whose
property is being set is the global object.

Comment 37

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate correct code for JSOP_BINDNAME.

Passes all trace-test.js and js/tests.

----

It is incorrect for BINDNAME to generate code to fetch fp->scopeChain
and supply that as the base object.  That might be the right answer at
recording time, but fp->scopeChain could have block objects added to
it off-trace, due to lazy cloning.  Instead, use a new function,
enclosingScopeChain, that finds the right object.  Try to preserve the
existing trace abort conditions.

Loosen assertion in test_property_cache, so that we can pass a direct
object not at the top of the scope chain, even though
js_FindPropertyHelper always creates cache entries that start there.

SETNAME need not abort recording if scopeChain has changed; it will
under normal circumstances.  Require only that the actual object whose
property is being set is the global object.

Comment 38

[Jim Blandy :jimb]

Attachment: Bug 469237: Avoid warnings in jstracer.cpp.

../jstracer.cpp: In member function ‘void TraceRecorder::guard(bool, nanojit::LIns*, nanojit::LIns*)’:
../jstracer.cpp:2301: warning: format ‘%p’ expects type ‘void*’, but argument 2 has type ‘nanojit::SideExit*’
../jstracer.cpp: In function ‘bool js_AttemptToExtendTree(JSContext*, VMSideExit*, VMSideExit*, jsbytecode*)’:
../jstracer.cpp:3624: warning: suggest parentheses around ‘&&’ within ‘||’

Comment 39

[Jim Blandy :jimb]

Attachment: [committed] Bug 469237: Assert that we never add properties to lexical blocks.

Comment 40

[Igor Bukanov]

(In reply to comment #37)
> Created an attachment (id=371728) [details]
> Bug 469237: Generate correct code for JSOP_BINDNAME.

If the proposal for the bug 462734 comment 19 is sound, then for the common case of the scope chain with single object the bindname recorder would not need to generate code to test the cache. Just a guard that the scope chain still points o that object would be enough.

Comment 41

[Igor Bukanov]

Comment on attachment 371733 [details] [diff] [review]
[committed] Bug 469237: Assert that we never add properties to lexical blocks.

>+        /* We should never add properties to lexical blocks.  */
>+        JS_ASSERT(OBJ_GET_CLASS(cx, obj) != &js_BlockClass);
>+

Note that this is true even with let blocks with eval('let a = 1'). In such eval the let statement behaves exactly as var adding properties to the var object and not to the let block.

Comment 42

[Igor Bukanov]

Comment on attachment 371728 [details] [diff] [review]
Bug 469237: Generate correct code for JSOP_BINDNAME.

>@@ -8721,23 +8852,26 @@ TraceRecorder::record_JSOP_POPN()
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_BINDNAME()
> {
>-    JSObject* obj = cx->fp->scopeChain;
>-    if (obj != globalObj)
>-        ABORT_TRACE("JSOP_BINDNAME crosses global scopes");
>-
>-    LIns* obj_ins = scopeChain();
>-    JSObject* obj2;
>+    JSObject *scope;
>+    LIns *scope_ins;
>+    if (!enclosingScopeChain(scope, scope_ins))
>+        return false;
>+
>+    if (scope != globalObj)
>+        ABORT_TRACE("JSOP_BINDNAME found non-global variable");
>+
>+    JSObject *pscope;
>     jsuword pcval;
>-    if (!test_property_cache(obj, obj_ins, obj2, pcval))
>+    if (!test_property_cache(scope, scope_ins, pscope, pcval))
>         return false;

With or without the patch this call to test_property_cache just reproduces the bug 462734 but in the tracer. The problem is that test_property_cache calls js_LookupPropertyWithFlags and JSOP_BINDNAME better not to do that for global object as it may cache the results with the wrong resolve hook flags.

The solution for this is simple. It is sufficient to replace test_property_cache with a guard that fp->scopeChain at the trace time is still the global object or that the global object still has the original shape to deal with eval calls off-trace that would add properties to the variable object of the function.

Comment 43

[Robert Sayre]

what is the status of this bug? does igor need to review attachment 371728 [details] [diff] [review], or is another patch needed?

Comment 44

[Igor Bukanov]

We need another patch that avoids any property lookup when recording JSOP_BINDNAME not to shift bug 462734 into the tracer.

Comment 45

[Jim Blandy :jimb]

I'm studying bug 462734, so I can understand why Igor's suggestion is correct.

Comment 46

[Jim Blandy :jimb]

When 487039 is fixed, we will be able to trust property cache hits in JSOP_BINDNAME.  I believe my fix for this bug will rely on that being the case.

Comment 47

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate proper code for JSOP_BINDNAME.

Let the interpreter compute the base object for the assignment.
Recognize uncacheable look-ups, and don't trace them.  For function
code, generate trace code to find the base object relative to the
scope object the function closed over.  For global code, only trace if
the base object is the global object.

Comment 48

[Jim Blandy :jimb]

(Passes js/tests and trace-test.js.)

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #46)
> When 487039 is fixed, we will be able to trust property cache hits in
> JSOP_BINDNAME.  I believe my fix for this bug will rely on that being the case.

So does that mean bug 487039 (which is not yet a blocker) blocks this bug? If so we should update the flags there. Do we know what the progress on that bug's fix is like?

Comment 50

[Jim Blandy :jimb]

Attachment: Bug 469237: Generate proper code for JSOP_BINDNAME.

[The prior patch was incomplete.  This patch has the missing pieces, and further relaxes restrictions on SETNAME.]

Let the interpreter compute the base object for the assignment.
Recognize uncacheable lookups, and don't trace them.  For function
code, generate trace code to find the base object relative to the
scope object the function closed over.  For global code, only trace if
the base object is the global object.  In setname, allow the base
object to be something other than the global object.

Comment 51

[Jim Blandy :jimb]

(In reply to comment #49)
> So does that mean bug 487039 (which is not yet a blocker) blocks this bug? If
> so we should update the flags there. Do we know what the progress on that bug's
> fix is like?

I may be able to get the change I need independent of that bug.

Comment 52

[Mike Shaver (:shaver emeritus)]

What will help you determine if you can get the change you need? Today is freeze, we once hoped, so more detail would be appreciated as we try to figure out what to do with the schedule.

Comment 53

[Jim Blandy :jimb]

(In reply to comment #52)
> What will help you determine if you can get the change you need? Today is
> freeze, we once hoped, so more detail would be appreciated as we try to figure
> out what to do with the schedule.

The change touches the same code as 487204; I had a patch that applied on top of that; now I need to rework the patch to apply without it.  Am doing so now.

Comment 54

[Andreas Gal :gal]

Comment on attachment 372905 [details] [diff] [review]
Bug 469237: Generate proper code for JSOP_BINDNAME.

>Bug 469237: Generate proper code for JSOP_BINDNAME.
>
>Let the interpreter compute the base object for the assignment.
>Recognize uncacheable lookups, and don't trace them.  For function
>code, generate trace code to find the base object relative to the
>scope object the function closed over.  For global code, only trace if
>the base object is the global object.  In setname, allow the base
>object to be something other than the global object.
>
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -3669,11 +3669,18 @@ js_Interpret(JSContext *cx)
>                     entry = NULL;
>                     LOAD_ATOM(0);
>                 }
>+                bool cacheable;
>                 id = ATOM_TO_JSID(atom);
>-                obj = js_FindIdentifierBase(cx, fp->scopeChain, id, entry);
>+                obj = js_FindIdentifierBase(cx, fp->scopeChain, id, entry,
>+                                            cacheable);

Fits on the same line under the new rules.

>                 if (!obj)
>                     goto error;
>+#ifdef JS_TRACER
>+                if (!cacheable && TRACE_RECORDER(cx))
>+                    js_AbortRecording(cx, "BINDNAME result uncacheable");
>+#endif
>             } while (0);
>+            TRACE_1(BindName, obj);
>             PUSH_OPND(OBJECT_TO_JSVAL(obj));
>           END_CASE(JSOP_BINDNAME)
> 
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -4089,7 +4089,7 @@ IsCacheableNonGlobalScope(JSObject *obj)
> 
> JSObject *
> js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
>-                      JSPropCacheEntry *entry)
>+                      JSPropCacheEntry *entry, bool &cacheable)
> {
>     /*
>      * This function should not be called for a global object or from the
>@@ -4122,13 +4122,16 @@ js_FindIdentifierBase(JSContext *cx, JSO
>                                  scopeIndex, protoIndex, pobj,
>                                  (JSScopeProperty *) prop, &entry);
>             JS_UNLOCK_OBJ(cx, pobj);
>+            cacheable = true;
>             return obj;
>         }
> 
>         /* Call and other cacheable objects always have a parent. */
>         obj = OBJ_GET_PARENT(cx, obj);
>-        if (!OBJ_GET_PARENT(cx, obj))
>+        if (!OBJ_GET_PARENT(cx, obj)) {
>+            cacheable = false;
>             return obj;
>+        }
>     }
> 
>     /* Loop until we find a property or reach the global object. */
>@@ -4152,6 +4155,7 @@ js_FindIdentifierBase(JSContext *cx, JSO
>             break;
>         obj = parent;
>     } while (OBJ_GET_PARENT(cx, obj));
>+    cacheable = false;
>     return obj;
> }
> 
>diff --git a/js/src/jsobj.h b/js/src/jsobj.h
>--- a/js/src/jsobj.h
>+++ b/js/src/jsobj.h
>@@ -682,7 +682,7 @@ js_FindProperty(JSContext *cx, jsid id, 
> 
> extern JS_REQUIRES_STACK JSObject *
> js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
>-                      JSPropCacheEntry *entry);
>+                      JSPropCacheEntry *entry, bool &cacheable);
> 
> extern JSObject *
> js_FindVariableScope(JSContext *cx, JSFunction **funp);
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -4973,6 +4973,22 @@ TraceRecorder::stackval(int n) const
>     return sp[n];
> }
> 
>+/*
>+ * Emit and return an ins whose value is fp->scopeChain.
>+ *
>+ * Note that fp->scopeChain may or may not include elements for the
>+ * function's internal features --- a call object, any cloned lexical
>+ * blocks, 'with' blocks, and so on --- depending on arbitrary
>+ * conditions in the program.  The interpreter adds Block objects to
>+ * fp->scopeChain only on demand (when we need to create a closure, or
>+ * enter a 'with' statement), so what one finds on fp->scopeChain can
>+ * vary from one trace entry to the next.  See the comments for
>+ * scopeChain in jsinterp.h for details.
>+ *
>+ * Because fp->scopeChain varies dynamically, it may not be the right
>+ * thing to pass to test_property_cache.  That function's comments
>+ * explain more.
>+ */
> JS_REQUIRES_STACK LIns*
> TraceRecorder::scopeChain() const
> {
>@@ -5870,6 +5886,33 @@ TraceRecorder::map_is_native(JSObjectMap
>     return true;
> }
> 
>+/*
>+ * Look up a property, and guard that the same lookup would find the
>+ * same property on trace.
>+ *
>+ * At recording time: Find the property referenced by the current
>+ * bytecode in OBJ; OBJ_INS should be an Lins for the expression that
>+ * yielded OBJ.  Set OBJ2 to the object actually holding the property
>+ * (OBJ or one of its delegates).  Fill a property cache entry, and
>+ * set PCVAL to its 'pcval' member, describing how to compute the
>+ * property's value.
>+ *
>+ * On trace: Emit guards equivalent to the tests we performed at
>+ * recording time to validate the property cache entry: check that the
>+ * relevant objects are native, and have the expected shape or
>+ * identity.  The guard code doesn't refer to the cache; the cache
>+ * entry's values are incorporated directly into the code.
>+ *
>+ * Note that, for identifier lookups, scopeChain() is probably not the
>+ * best thing to use for OBJ_INS.  Because SpiderMonkey clones lexical
>+ * block objects onto fp->scopeChain lazily (see the comments in
>+ * jsinterp.h about JSStackFrame::scopeChain and blockChain), the
>+ * value computed by scopeChain()'s ins can change depending on the
>+ * code path executed off-trace; it is not always the same at a given
>+ * code location.  The guards this function generates will likely fail
>+ * unnecessarily if we record when OBJ has one value, and then run the
>+ * trace when OBJ_INS yields another.
>+ */
> JS_REQUIRES_STACK bool
> TraceRecorder::test_property_cache(JSObject* obj, LIns* obj_ins, JSObject*& obj2, jsuword& pcval)
> {
>@@ -8734,23 +8777,78 @@ TraceRecorder::record_JSOP_POPN()
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_BINDNAME()
> {
>-    JSObject* obj = cx->fp->scopeChain;
>-    if (obj != globalObj)
>-        ABORT_TRACE("JSOP_BINDNAME crosses global scopes");
>-
>-    LIns* obj_ins = scopeChain();
>-    JSObject* obj2;
>-    jsuword pcval;
>-    if (!test_property_cache(obj, obj_ins, obj2, pcval))
>-        return false;
>-
>-    if (PCVAL_IS_NULL(pcval))
>-        ABORT_TRACE("JSOP_BINDNAME is trying to add a new property");
>-
>-    if (obj2 != obj)
>-        ABORT_TRACE("JSOP_BINDNAME found a non-direct property on the global object");
>-
>-    stack(0, obj_ins);
>+    // The bulk of the work is done in record_BindName.
>+    return true;
>+}
>+
>+/*
>+ * If ancestor is the n'th parent of scope, return an LIns that
>+ * computes the n'th parent of scope_ins.  If ancestor is not, in fact,
>+ * an ancestor of scope, then return NULL.
>+ */
>+LIns *
>+TraceRecorder::findAncestor(JSObject *ancestor, JSObject *scope,
>+                            LIns *scope_ins)

one line

>+{
>+    while (scope != ancestor) {
>+        scope = OBJ_GET_PARENT(cx, scope);
>+        if (!scope)
>+            return NULL;
>+        scope_ins = stobj_get_fslot(scope_ins, JSSLOT_PARENT);
>+    }
>+    return scope_ins;
>+}
>+
>+JS_REQUIRES_STACK bool
>+TraceRecorder::record_BindName(JSObject *pbase)
>+{
>+    JSStackFrame *fp = cx->fp;
>+    LIns *pbase_ins;
>+
>+    // JSOP_BINDNAME calls this TraceRecorder method only when it
>+    // found a base object and could cache the search.  Thus, the
>+    // scope chain elements it passed over are well-behaved, in the
>+    // sense of IsCacheableNonGlobalScope.
>+
>+    // If the base object is the global object, traces are specialized
>+    // to a particular global object, so we can just hard-code that.
>+    if (pbase == globalObj)
>+        pbase_ins = INS_CONSTPTR(globalObj);

{ } since else part is not a single-line statement (don't look at me, I didn't make up these rules)

>+

extra space here is confusing

>+    // Are we tracing function code?
>+    else if (fp->callee) {
>+        JSObject *closure = OBJ_GET_PARENT(cx, fp->callee);
>+
>+        // If the function is closed over the global object, then
>+        // JSOP_BINDNAME should have choosen that as the base object.
>+        JS_ASSERT(closure != globalObj);
>+        
>+        // On trace, we can follow the same number of links up
>+        // from the environment the function closed over, and find
>+        // the correct base for us.

same line for all of these

also, (int) offsetof otherwise you get a warning on windows I think

>+        LIns *fp_ins
>+            = lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, fp));
>+        LIns *callee_ins
>+            = lir->insLoad(LIR_ldp, fp_ins, offsetof(JSStackFrame, callee));
>+        LIns *scope_ins
>+            = stobj_get_fslot(callee_ins, JSSLOT_PARENT);
>+        pbase_ins = findAncestor(pbase, closure, scope_ins);
>+
>+        // JSOP_BINDNAME should always choose a base somewhere on the 
>+        // function's scope chain.
>+        JS_ASSERT(pbase_ins);
>+    } else {
>+        // In global code, aside from globalObj, we have no fixed
>+        // reference point from which we can find pbase: lexical block
>+        // and call objects get placed on fp->scopeChain lazily, so it
>+        // may refer to different things by the time we enter the
>+        // trace.  So unless pbase is globalObj (whose shape we've
>+        // already guarded on), we can't record.
>+        if (pbase != globalObj)
>+            ABORT_TRACE("JSOP_BINDNAME found non-global variable in global code");
>+        pbase_ins = INS_CONSTPTR(globalObj);
>+    }
>+    stack(0, pbase_ins);
>     return true;
> }
> 
>@@ -8760,14 +8858,6 @@ TraceRecorder::record_JSOP_SETNAME()
>     jsval& l = stackval(-2);
>     JS_ASSERT(!JSVAL_IS_PRIMITIVE(l));
> 
>-    /*
>-     * Trace cases that are global code or in lightweight functions scoped by
>-     * the global object only.
>-     */
>-    JSObject* obj = JSVAL_TO_OBJECT(l);
>-    if (obj != cx->fp->scopeChain || obj != globalObj)
>-        ABORT_TRACE("JSOP_SETNAME left operand is not the global object");
>-
>     // The rest of the work is in record_SetPropHit.
>     return true;
> }
>diff --git a/js/src/jstracer.h b/js/src/jstracer.h
>--- a/js/src/jstracer.h
>+++ b/js/src/jstracer.h
>@@ -569,6 +569,9 @@ class TraceRecorder : public avmplus::GC
> 
>     JS_REQUIRES_STACK jsatomid getFullIndex(ptrdiff_t pcoff = 0);
> 
>+    nanojit::LIns *findAncestor(JSObject *ancestor, JSObject *scope,
>+                                nanojit::LIns *scope_ins);
>+

probably one line too

> public:
>     JS_REQUIRES_STACK
>     TraceRecorder(JSContext* cx, VMSideExit*, nanojit::Fragment*, TreeInfo*,
>@@ -607,6 +610,7 @@ public:
>     JS_REQUIRES_STACK bool record_SetPropMiss(JSPropCacheEntry* entry);
>     JS_REQUIRES_STACK bool record_DefLocalFunSetSlot(uint32 slot, JSObject* obj);
>     JS_REQUIRES_STACK bool record_FastNativeCallComplete();
>+    JS_REQUIRES_STACK bool record_BindName(JSObject *base);
> 
>     bool wasDeepAborted() { return deepAborted; }
>     TreeInfo* getTreeInfo() { return treeInfo; }

Comment 55

[Andreas Gal :gal]

Could you run sunspider with this? Just to double check that we don't regress perf.

Comment 56

[Brendan Eich [:brendan]]

Please cut down patch excerpts, avoid miles of useless over-citation!

The copy-pasted code from long-standing 80 column code is not a ripe target for expansion to 100 (gross including newline) wonderland, IMHO.

Igor in particular is old-sk00l and I try to respect that.

/be

Comment 57

[Jim Blandy :jimb]

Attachment: Bug 469237: Only generate legitimate cache entries in js_FindPropertyHelper.

Comment 58

[Igor Bukanov]

Comment on attachment 372985 [details] [diff] [review]
Bug 469237: Only generate legitimate cache entries in js_FindPropertyHelper.

>+
>+    // We can cache hits in the first outer object, but none further out.

Nit: for consistency, use /* comments.

>+    JSObject *first_outer = obj;
>+
>+    for (; ; scopeIndex++) {
>         if (obj->map->ops->lookupProperty == js_LookupProperty) {
>             protoIndex =
>                 js_LookupPropertyWithFlags(cx, obj, id, 

Here only the first iteration of the loop can ever call js_FillPropertyCache. So just check for js_LookupProperty and do the caching before the loop.

Comment 59

[Igor Bukanov]

Comment on attachment 372905 [details] [diff] [review]
Bug 469237: Generate proper code for JSOP_BINDNAME.

> JSObject *
> js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
>-                      JSPropCacheEntry *entry)
>+                      JSPropCacheEntry *entry, bool &cacheable)

Does this compiles? LiveConnect C sources may include jsobj.h.

Comment 60

[Brendan Eich [:brendan]]

Out parameters for functions prototyped in most .h files should be pointer-typed, with actuals passed explicitly by address, for now.

/be

Comment 61

[Jim Blandy :jimb]

(In reply to comment #55)
> Could you run sunspider with this? Just to double check that we don't regress
> perf.

** TOTAL **:           *1.079x as slow*  1287.3ms +/- 1.1%   1388.5ms +/- 1.1% 
   significant

It does regress.  Let me try something.

Comment 62

[Igor Bukanov]

(In reply to comment #56)
> The copy-pasted code from long-standing 80 column code is not a ripe target for
> expansion to 100 (gross including newline) wonderland, IMHO.
> 
> Igor in particular is old-sk00l and I try to respect that.

I repeat why 80 chars is still relevant. 

1. DPI of monitors has not increased significantly for the last 10 (or 15 years). They are still around 96-120. That means that a very small font is still unreadable on the screen when on paper it would look nice and easy to read. Just consider how any high math sucks on the screen and how nice it is in text books.

2. Human eye has rather narrow angle of conscious perception of static text. Anything beyond that angle requires to move either the eyes the neck.

What I have found is that on a notebook screen with 120 DPI I can make the font small enough so 80-char width fits for me that angle of eyesight. With 100 chars this is no longer feasible which translates in extra tiresome movement of either the neck or eyesight.

Comment 63

[Andreas Gal :gal]

If we want to discuss line length rules, lets make a separate bug for that.

Comment 64

[Andreas Gal :gal]

(Note, I plus-ed the patch. I don't really care about formatting issues, in particularly the line length issue. I am fine with whatever nits you accept or ignore.)

Comment 65

[Brendan Eich [:brendan]]

Back to the bug: any propcache.stats or TRACEMONKEY=stats diffs of note due to the patch? Or the old `TRACEMONKEY=verbose ./js ... | grep -i abort` diff game.

/be

Comment 66

[Jim Blandy :jimb]

Attachment: Bug 469237: Only trace where BINDNAME will choose the global object.

SunSpider never excercises the non-global case.  For more
sophisticated code, upvar2 makes references up the scope chain less
common, making this simplification less expensive.

Performance is less bad:

** TOTAL **:           *1.049x as slow*  1244.7ms +/- 1.1%   1305.2ms +/- 1.2%     significant

Comment 67

[Andreas Gal :gal]

Thats still way beyond our acceptable threshold to take it. Can you post the entire result page?

Comment 68

[Brendan Eich [:brendan]]

SunSpider doesn't use let at all, and catch doesn't matter (we don't trace exception handling). What's being done differently? There must be a different jitstats abort count, or more and shorter traces being triggered, or fewer traces of any size triggered, or something like that.

/be

Comment 69

[Andreas Gal :gal]

I am pretty sure the complete stats of sunspider will tell. I bet we don't trace something we used to trace before. 60ms hit is enormous.

Comment 70

[Jim Blandy :jimb]

Full results:

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.049x as slow*  1244.7ms +/- 1.1%   1305.2ms +/- 1.2%     significant

=============================================================================

  3d:                  *1.058x as slow*   186.0ms +/- 2.9%    196.8ms +/- 3.0%     significant
    cube:              *1.129x as slow*    50.6ms +/- 3.5%     57.1ms +/- 5.5%     significant
    morph:             ??                  44.1ms +/- 8.4%     46.5ms +/- 8.3%     not conclusive: might be *1.053x as slow*
    raytrace:          ??                  91.3ms +/- 3.7%     93.2ms +/- 3.5%     not conclusive: might be *1.021x as slow*

  access:              *1.047x as slow*   161.2ms +/- 1.3%    168.7ms +/- 1.9%     significant
    binary-trees:      *1.058x as slow*    41.4ms +/- 1.4%     43.8ms +/- 4.9%     significant
    fannkuch:          ??                  74.8ms +/- 1.9%     76.1ms +/- 2.9%     not conclusive: might be *1.018x as slow*
    nbody:             *1.116x as slow*    31.9ms +/- 1.0%     35.6ms +/- 6.0%     significant
    nsieve:            ??                  13.1ms +/- 2.1%     13.2ms +/- 5.1%     not conclusive: might be *1.006x as slow*

  bitops:              ??                  42.9ms +/- 1.8%     44.0ms +/- 4.3%     not conclusive: might be *1.027x as slow*
    3bit-bits-in-byte: -                    1.8ms +/- 6.7%      1.8ms +/- 10.1% 
    bits-in-byte:      ??                   9.2ms +/- 2.8%      9.4ms +/- 8.1%     not conclusive: might be *1.026x as slow*
    bitwise-and:       ??                   2.5ms +/- 5.8%      2.6ms +/- 7.9%     not conclusive: might be *1.065x as slow*
    nsieve-bits:       ??                  29.5ms +/- 1.5%     30.3ms +/- 5.4%     not conclusive: might be *1.026x as slow*

  controlflow:         ??                  41.1ms +/- 3.0%     41.4ms +/- 4.8%     not conclusive: might be *1.009x as slow*
    recursive:         ??                  41.1ms +/- 3.0%     41.4ms +/- 4.8%     not conclusive: might be *1.009x as slow*

  crypto:              *1.054x as slow*    75.6ms +/- 1.8%     79.7ms +/- 4.3%     significant
    aes:               ??                  42.4ms +/- 2.9%     43.4ms +/- 5.7%     not conclusive: might be *1.024x as slow*
    md5:               *1.088x as slow*    23.8ms +/- 1.4%     25.9ms +/- 7.5%     significant
    sha1:              *1.109x as slow*     9.4ms +/- 1.5%     10.4ms +/- 7.8%     significant

  date:                *1.078x as slow*   195.8ms +/- 1.7%    211.0ms +/- 2.3%     significant
    format-tofte:      *1.076x as slow*    75.7ms +/- 2.6%     81.5ms +/- 3.6%     significant
    format-xparb:      *1.078x as slow*   120.1ms +/- 1.7%    129.5ms +/- 2.6%     significant

  math:                *1.097x as slow*    58.3ms +/- 1.5%     64.0ms +/- 4.5%     significant
    cordic:            *1.080x as slow*    28.5ms +/- 1.2%     30.7ms +/- 5.1%     significant
    partial-sums:      *1.137x as slow*    22.9ms +/- 2.9%     26.1ms +/- 8.9%     significant
    spectral-norm:     *1.038x as slow*     6.9ms +/- 1.4%      7.1ms +/- 3.5%     significant

  regexp:              *1.040x as slow*    52.6ms +/- 1.2%     54.7ms +/- 4.6%     significant
    dna:               *1.040x as slow*    52.6ms +/- 1.2%     54.7ms +/- 4.6%     significant

  string:              *1.032x as slow*   431.2ms +/- 1.3%    444.8ms +/- 1.3%     significant
    base64:            ??                  19.8ms +/- 4.4%     20.3ms +/- 6.1%     not conclusive: might be *1.023x as slow*
    fasta:             *1.030x as slow*    91.7ms +/- 1.6%     94.4ms +/- 3.4%     significant
    tagcloud:          ??                 127.4ms +/- 1.9%    129.3ms +/- 1.9%     not conclusive: might be *1.015x as slow*
    unpack-code:       *1.049x as slow*   154.0ms +/- 1.2%    161.6ms +/- 1.6%     significant
    validate-input:    ??                  38.2ms +/- 5.8%     39.1ms +/- 4.9%     not conclusive: might be *1.023x as slow*

Comment 71

[Jim Blandy :jimb]

For an entire pass through SunSpider, below is what grep -i, diff produces.  Note that crypto-aes is not reported as being significantly slower in the SM results.

--- /home/jimb/mc/wtm/js/src/pop.aborts~	2009-04-15 17:55:36.000000000 -0700
+++ /home/jimb/mc/wtm/js/src/push.aborts~	2009-04-15 17:55:43.000000000 -0700
@@ -74,17 +74,18 @@
 Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:76@8: No compatible inner tree.
 Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:76@8: Inner tree is trying to stabilize, abort outer recording.
 Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:34@290: No compatible inner tree.
+deep abortentering REGEXP trace at tests/crypto-aes.js:279@12, code: 0xb7be6f93
+Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:279@15: deep abort requested.
 Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:194@350: No compatible inner tree.
 Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:195@397: No compatible inner tree.
-deep abortentering REGEXP trace at tests/crypto-aes.js:279@12, code: 0xb7b22f93
-Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:279@15: deep abort requested.
-deep abortentering REGEXP trace at tests/crypto-aes.js:279@12, code: 0xb7b22f93
-Abort recording of tree tests/crypto-aes.js:191@341 at tests/crypto-aes.js:279@15: deep abort requested.
 Abort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:254@301: No compatible inner tree.
 deep abortAbort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:283@15: deep abort requested.
-deep abortLooking for compat peer 262@450, from 0xb1487e0 (ip: 0xacfae8e)
+deep abortAbort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:283@15: deep abort requested.
+deep abortLooking for compat peer 262@450, from 0x9726f58 (ip: 0x92bee8e)
 Abort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:254@301: No compatible inner tree.
 Abort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:255@350: No compatible inner tree.
+deep abortAbort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:283@15: deep abort requested.
+deep abortAbort recording of tree tests/crypto-aes.js:252@292 at tests/crypto-aes.js:283@15: deep abort requested.
 abort: 8339: can't see through hole in dense array
 Abort recording of tree tests/crypto-sha1.js:61@155 at tests/crypto-sha1.js:63@181: getelem.
 abort: 8339: can't see through hole in dense array

Comment 72

[Jim Blandy :jimb]

And of course, that none of the aborts come from record_JSOP_BINDNAME.

Comment 73

[Andreas Gal :gal]

How many runs did you use? I usually use 50 for stable results.

Comment 74

[Jim Blandy :jimb]

Here's with 200 runs, exited Emacs, Firefox and Thunderbird.

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.060x as slow*  1256.9ms +/- 0.9%   1331.9ms +/- 0.4%     significant

=============================================================================

  3d:                  *1.059x as slow*   183.9ms +/- 1.4%    194.8ms +/- 1.0%     significant
    cube:              *1.087x as slow*    50.1ms +/- 2.2%     54.4ms +/- 2.7%     significant
    morph:             *1.084x as slow*    42.2ms +/- 4.2%     45.7ms +/- 4.1%     significant
    raytrace:          *1.033x as slow*    91.7ms +/- 1.5%     94.7ms +/- 1.6%     significant

  access:              *1.066x as slow*   165.1ms +/- 1.1%    175.9ms +/- 1.0%     significant
    binary-trees:      *1.083x as slow*    42.8ms +/- 2.2%     46.4ms +/- 2.4%     significant
    fannkuch:          *1.045x as slow*    75.4ms +/- 1.5%     78.8ms +/- 1.6%     significant
    nbody:             *1.104x as slow*    33.3ms +/- 1.9%     36.8ms +/- 2.7%     significant
    nsieve:            ??                  13.6ms +/- 3.0%     14.0ms +/- 3.3%     not conclusive: might be *1.031x as slow*

  bitops:              *1.046x as slow*    43.8ms +/- 1.7%     45.8ms +/- 2.4%     significant
    3bit-bits-in-byte: ??                   1.9ms +/- 4.3%      2.0ms +/- 4.5%     not conclusive: might be *1.047x as slow*
    bits-in-byte:      *1.075x as slow*     9.3ms +/- 2.8%     10.0ms +/- 4.4%     significant
    bitwise-and:       ??                   2.6ms +/- 4.0%      2.6ms +/- 4.5%     not conclusive: might be *1.021x as slow*
    nsieve-bits:       *1.040x as slow*    29.9ms +/- 1.8%     31.1ms +/- 2.7%     significant

  controlflow:         *1.043x as slow*    40.8ms +/- 1.9%     42.5ms +/- 2.3%     significant
    recursive:         *1.043x as slow*    40.8ms +/- 1.9%     42.5ms +/- 2.3%     significant

  crypto:              *1.061x as slow*    77.1ms +/- 1.5%     81.8ms +/- 1.5%     significant
    aes:               *1.047x as slow*    43.1ms +/- 2.2%     45.1ms +/- 2.3%     significant
    md5:               *1.074x as slow*    24.5ms +/- 2.6%     26.4ms +/- 3.1%     significant
    sha1:              *1.095x as slow*     9.4ms +/- 1.7%     10.3ms +/- 3.6%     significant

  date:                *1.068x as slow*   201.1ms +/- 1.1%    214.7ms +/- 0.9%     significant
    format-tofte:      *1.062x as slow*    77.6ms +/- 1.5%     82.5ms +/- 1.7%     significant
    format-xparb:      *1.071x as slow*   123.4ms +/- 1.1%    132.2ms +/- 0.9%     significant

  math:                *1.050x as slow*    60.7ms +/- 1.5%     63.7ms +/- 1.9%     significant
    cordic:            *1.058x as slow*    29.9ms +/- 2.4%     31.6ms +/- 3.1%     significant
    partial-sums:      *1.042x as slow*    23.6ms +/- 2.3%     24.6ms +/- 3.0%     significant
    spectral-norm:     *1.038x as slow*     7.2ms +/- 2.4%      7.5ms +/- 3.2%     significant

  regexp:              *1.045x as slow*    53.2ms +/- 1.6%     55.6ms +/- 2.0%     significant
    dna:               *1.045x as slow*    53.2ms +/- 1.6%     55.6ms +/- 2.0%     significant

  string:              *1.060x as slow*   431.3ms +/- 1.0%    457.0ms +/- 0.4%     significant
    base64:            *1.073x as slow*    19.8ms +/- 2.4%     21.3ms +/- 3.4%     significant
    fasta:             *1.057x as slow*    90.6ms +/- 1.3%     95.8ms +/- 1.2%     significant
    tagcloud:          *1.053x as slow*   128.0ms +/- 1.0%    134.8ms +/- 0.6%     significant
    unpack-code:       *1.064x as slow*   154.7ms +/- 1.1%    164.6ms +/- 0.7%     significant
    validate-input:    *1.064x as slow*    38.1ms +/- 2.0%     40.5ms +/- 2.5%     significant

Comment 75

[Jim Blandy :jimb]

Looking at the exits in the verbose output, the patch changes which exits are taken; many of the exits affected are of this sort:

+leaving trace at tests/crypto-aes.js:76@74, op=lt, lr=<hex>, exitType=3, sp=2, calldepth=0, cycles=<N>

exitType=3 means LOOP_EXIT, and the opcodes are 'lt', which means that the patch is causing loops to exit earlier --- this patch should have no effect on that.  What's odd is that all the exits affected by the patch are in crypto-aes.js --- which never traces a BINDNAME opcode!

Comment 76

[Jim Blandy :jimb]

Oh... crypto-aes.js actually calls (new Date()).getTime() and then computes with that.  So it would make sense that only crypto-aes would show this sort of instability.  So that doesn't mean anything.

Comment 77

[Igor Bukanov]

Comment on attachment 372985 [details] [diff] [review]
Bug 469237: Only generate legitimate cache entries in js_FindPropertyHelper.

The patch needs an update due to changes from the bug 487204.

Comment 78

[Igor Bukanov]

Comment on attachment 373016 [details] [diff] [review]
Bug 469237: Only trace where BINDNAME will choose the global object.

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
> TraceRecorder::record_JSOP_BINDNAME()
> {
>+
>+    if (fp->callee) {
>+        scope = OBJ_GET_PARENT(cx, fp->callee);

This assumes that the trace is not recorded for functions with Call objects. An assert should enforce this. And if there is no call object, then OBJ_GET_PARENT(cx, fp->callee) must match fp->scopeChain.

>+    } else {
>+        scope = fp->scopeChain;
>+
>+        // Skip objects whose properties we don't use BINDNAME to find.

The comment is not true: bindname is used to access the let variables from inside eval.

Comment 79

[Jim Blandy :jimb]

(In reply to comment #76)
> Oh... crypto-aes.js actually calls (new Date()).getTime() and then computes
> with that.  So it would make sense that only crypto-aes would show this sort of
> instability.  So that doesn't mean anything.

Indeed, replacing the (new Date()).getTime() with a suitable constant gives verbose output with identical 'exiting' lists and final statistics.

Comment 80

[Andreas Gal :gal]

And performance?

Comment 81

[Brendan Eich [:brendan]]

SunSpider and other supposedly reproducible performance benchmarks shouldn't have such over ND -- or so I think. Opinions?

/be

Comment 82

[Brendan Eich [:brendan]]

s/over/overt/

/be

Comment 83

[Andreas Gal :gal]

jim, without the getTime() insanity, is there still a perf difference?

Comment 84

[Jim Blandy :jimb]

(In reply to comment #78)
> (From update of attachment 373016 [details] [diff] [review])
> >diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
> > TraceRecorder::record_JSOP_BINDNAME()
> > {
> >+
> >+    if (fp->callee) {
> >+        scope = OBJ_GET_PARENT(cx, fp->callee);
> 
> This assumes that the trace is not recorded for functions with Call objects. An
> assert should enforce this.

I don't think it makes that assumption.  As I understand it, BINDNAME will
never find a binding on a function's own call object (although it may on call
objects the function has closed over) --- unless it is within a 'with', in
which case we won't JIT.  Functions refer to variables covered by their own
call objects using *LOCAL opcodes, never BINDNAME.  I think this is necessary
to allow them to work correctly when the call object has been omitted.

function f(x) {
  var a = x;
  g = function() { return a; };
  a=a*2;
  return a;
}
// *** disassembly: f
// flags: HEAVYWEIGHT
// main:
// 00000:  getarg 0
// 00003:  setlocal 0
// 00006:  pop
// 00007:  bindname "g"
// 00010:  lambda (function () {return a;})
// 00013:  setname "g"
// 00016:  pop
// 00017:  getlocal 0
// 00020:  int8 2
// 00022:  mul
// 00023:  setlocal 0
// 00026:  pop
// 00027:  getlocal 0
// 00030:  return
// 00031:  stop

(If my argument is correct, it certainly needs a comment.)

Comment 85

[Jim Blandy :jimb]

(In reply to comment #81)
> SunSpider and other supposedly reproducible performance benchmarks shouldn't
> have such over ND -- or so I think. Opinions?

If the purpose of a benchmark is to make a measurement, and the nondeterminism adds noise to that measurement, then it's a bad thing, I would say.

Comment 86

[Igor Bukanov]

(In reply to comment #84)
> I don't think it makes that assumption.  As I understand it, BINDNAME will
> never find a binding on a function's own call object (although it may on call
> objects the function has closed over) --- unless it is within a 'with', in
> which case we won't JIT.

eval("var v") inside a function adds a variable to the Call object. After that BINDNAME for v must find that added v inside the Call object.




  Functions refer to variables covered by their own
> call objects using *LOCAL opcodes, never BINDNAME.  I think this is necessary
> to allow them to work correctly when the call object has been omitted.
> 
> function f(x) {
>   var a = x;
>   g = function() { return a; };
>   a=a*2;
>   return a;
> }
> // *** disassembly: f
> // flags: HEAVYWEIGHT
> // main:
> // 00000:  getarg 0
> // 00003:  setlocal 0
> // 00006:  pop
> // 00007:  bindname "g"
> // 00010:  lambda (function () {return a;})
> // 00013:  setname "g"
> // 00016:  pop
> // 00017:  getlocal 0
> // 00020:  int8 2
> // 00022:  mul
> // 00023:  setlocal 0
> // 00026:  pop
> // 00027:  getlocal 0
> // 00030:  return
> // 00031:  stop
> 
> (If my argument is correct, it certainly needs a comment.)

Comment 87

[Jim Blandy :jimb]

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.031x as slow*  1351.5ms +/- 0.7%   1393.1ms +/- 0.7%     significant

=============================================================================

  3d:                  ??                 195.1ms +/- 1.0%    197.8ms +/- 0.9%     not conclusive: might be *1.014x as slow*
    cube:              1.030x as fast      52.6ms +/- 2.3%     51.0ms +/- 1.3%     significant
    morph:             ??                  45.4ms +/- 4.0%     46.5ms +/- 4.0%     not conclusive: might be *1.026x as slow*
    raytrace:          *1.032x as slow*    97.1ms +/- 1.3%    100.2ms +/- 1.0%     significant

  access:              *1.046x as slow*   181.1ms +/- 1.0%    189.4ms +/- 1.0%     significant
    binary-trees:      *1.053x as slow*    46.1ms +/- 2.3%     48.5ms +/- 1.1%     significant
    fannkuch:          *1.056x as slow*    82.4ms +/- 1.5%     87.0ms +/- 1.0%     significant
    nbody:             ??                  37.6ms +/- 2.3%     38.0ms +/- 1.0%     not conclusive: might be *1.011x as slow*
    nsieve:            *1.054x as slow*    15.0ms +/- 3.4%     15.8ms +/- 1.8%     significant

  bitops:              *1.078x as slow*    46.3ms +/- 2.1%     50.0ms +/- 1.0%     significant
    3bit-bits-in-byte: ??                   1.9ms +/- 4.9%      1.9ms +/- 6.0%     not conclusive: might be *1.016x as slow*
    bits-in-byte:      *1.065x as slow*     9.8ms +/- 3.9%     10.4ms +/- 1.3%     significant
    bitwise-and:       *1.089x as slow*     2.5ms +/- 4.8%      2.7ms +/- 2.8%     significant
    nsieve-bits:       *1.085x as slow*    32.2ms +/- 2.1%     34.9ms +/- 1.0%     significant

  controlflow:         *1.029x as slow*    44.3ms +/- 2.1%     45.6ms +/- 0.9%     significant
    recursive:         *1.029x as slow*    44.3ms +/- 2.1%     45.6ms +/- 0.9%     significant

  crypto:              *1.044x as slow*    82.4ms +/- 1.5%     86.0ms +/- 0.9%     significant
    aes:               *1.046x as slow*    45.6ms +/- 2.2%     47.7ms +/- 0.9%     significant
    md5:               *1.037x as slow*    26.5ms +/- 3.1%     27.5ms +/- 1.0%     significant
    sha1:              *1.053x as slow*    10.3ms +/- 3.2%     10.9ms +/- 1.7%     significant

  date:                ??                 222.3ms +/- 0.9%    223.6ms +/- 0.8%     not conclusive: might be *1.006x as slow*
    format-tofte:      ??                  83.3ms +/- 1.5%     83.7ms +/- 0.8%     not conclusive: might be *1.006x as slow*
    format-xparb:      ??                 139.0ms +/- 0.9%    139.9ms +/- 0.8%     not conclusive: might be *1.006x as slow*

  math:                ??                  61.5ms +/- 1.6%     62.5ms +/- 0.8%     not conclusive: might be *1.016x as slow*
    cordic:            *1.036x as slow*    30.4ms +/- 2.5%     31.5ms +/- 0.9%     significant
    partial-sums:      -                   23.8ms +/- 2.0%     23.4ms +/- 0.8% 
    spectral-norm:     ??                   7.4ms +/- 3.0%      7.6ms +/- 1.3%     not conclusive: might be *1.031x as slow*

  regexp:              *1.076x as slow*    52.9ms +/- 1.8%     56.9ms +/- 0.7%     significant
    dna:               *1.076x as slow*    52.9ms +/- 1.8%     56.9ms +/- 0.7%     significant

  string:              *1.034x as slow*   465.6ms +/- 0.9%    481.3ms +/- 0.6%     significant
    base64:            ??                  21.5ms +/- 3.2%     22.2ms +/- 1.0%     not conclusive: might be *1.032x as slow*
    fasta:             *1.073x as slow*    94.7ms +/- 1.3%    101.6ms +/- 0.7%     significant
    tagcloud:          ??                 139.0ms +/- 1.2%    140.8ms +/- 0.6%     not conclusive: might be *1.013x as slow*
    unpack-code:       *1.042x as slow*   168.7ms +/- 1.0%    175.8ms +/- 0.6%     significant
    validate-input:    -                   41.8ms +/- 2.7%     41.0ms +/- 0.6%

Comment 88

[Jim Blandy :jimb]

So, last night, I got the results below, comparing two SunSpider passes, 1000 runs each.  There was change to the patch, but this set of runs shows only a .5% performance hit.

I changed my timing script to run with the patch applied first, and then without the patch --- and observed a 2.1% "speed-up" from the patch.

So my timing rig has a consistent bias against whatever I run second.  I don't think there's any performance problem here.  I'd like to get some confirmation from someone else.

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.005x as slow*  1381.7ms +/- 0.3%   1388.2ms +/- 0.1%     significant

=============================================================================

  3d:                  *1.014x as slow*   200.1ms +/- 0.4%    202.9ms +/- 0.4%     significant
    cube:              *1.020x as slow*    54.7ms +/- 1.1%     55.8ms +/- 1.1%     significant
    morph:             *1.035x as slow*    45.3ms +/- 1.7%     46.8ms +/- 1.5%     significant
    raytrace:          -                  100.2ms +/- 0.7%    100.2ms +/- 0.6% 

  access:              1.007x as fast     185.9ms +/- 0.5%    184.5ms +/- 0.4%     significant
    binary-trees:      1.041x as fast      52.3ms +/- 1.1%     50.2ms +/- 1.1%     significant
    fannkuch:          ??                  81.1ms +/- 0.7%     81.9ms +/- 0.7%     not conclusive: might be *1.010x as slow*
    nbody:             -                   37.8ms +/- 1.4%     37.6ms +/- 1.4% 
    nsieve:            ??                  14.7ms +/- 1.7%     14.7ms +/- 1.7%     not conclusive: might be *1.002x as slow*

  bitops:              1.017x as fast      48.0ms +/- 1.1%     47.2ms +/- 1.1%     significant
    3bit-bits-in-byte: -                    1.9ms +/- 2.5%      1.8ms +/- 2.5% 
    bits-in-byte:      ??                   9.5ms +/- 1.8%      9.6ms +/- 1.7%     not conclusive: might be *1.010x as slow*
    bitwise-and:       -                    2.7ms +/- 2.2%      2.7ms +/- 2.0% 
    nsieve-bits:       1.025x as fast      33.9ms +/- 1.3%     33.1ms +/- 1.3%     significant

  controlflow:         *1.087x as slow*    45.0ms +/- 1.2%     48.9ms +/- 1.1%     significant
    recursive:         *1.087x as slow*    45.0ms +/- 1.2%     48.9ms +/- 1.1%     significant

  crypto:              -                   87.2ms +/- 2.3%     86.4ms +/- 0.6% 
    aes:               -                   48.6ms +/- 1.2%     48.1ms +/- 1.1% 
    md5:               -                   28.0ms +/- 7.1%     27.4ms +/- 1.5% 
    sha1:              *1.028x as slow*    10.6ms +/- 1.8%     10.9ms +/- 1.8%     significant

  date:                ??                 222.7ms +/- 0.4%    223.5ms +/- 0.3%     not conclusive: might be *1.004x as slow*
    format-tofte:      ??                  83.2ms +/- 0.7%     83.9ms +/- 0.7%     not conclusive: might be *1.008x as slow*
    format-xparb:      ??                 139.4ms +/- 0.3%    139.6ms +/- 0.2%     not conclusive: might be *1.001x as slow*

  math:                ??                  64.0ms +/- 0.9%     64.7ms +/- 0.9%     not conclusive: might be *1.010x as slow*
    cordic:            *1.026x as slow*    31.5ms +/- 1.4%     32.3ms +/- 1.4%     significant
    partial-sums:      -                   25.1ms +/- 1.5%     24.9ms +/- 1.4% 
    spectral-norm:     ??                   7.4ms +/- 1.5%      7.5ms +/- 1.5%     not conclusive: might be *1.005x as slow*

  regexp:              -                   54.5ms +/- 1.0%     54.3ms +/- 1.0% 
    dna:               -                   54.5ms +/- 1.0%     54.3ms +/- 1.0% 

  string:              ??                 474.3ms +/- 0.3%    475.8ms +/- 0.2%     not conclusive: might be *1.003x as slow*
    base64:            1.026x as fast      21.9ms +/- 1.7%     21.3ms +/- 1.5%     significant
    fasta:             ??                  99.6ms +/- 0.6%    100.2ms +/- 0.5%     not conclusive: might be *1.006x as slow*
    tagcloud:          *1.011x as slow*   140.9ms +/- 0.3%    142.5ms +/- 0.2%     significant
    unpack-code:       ??                 169.9ms +/- 0.4%    170.2ms +/- 0.3%     not conclusive: might be *1.002x as slow*
    validate-input:    -                   42.1ms +/- 1.3%     41.5ms +/- 1.2%

Comment 89

[Andreas Gal :gal]

No perf regression.

Comment 90

[Mike Beltzner [:beltzner, not reading bugmail]]

So are we ready to re-land?

Comment 91

[Andreas Gal :gal]

Yeah, this should happen shortly.

Comment 92

[Jim Blandy :jimb]

Attachment: Bug 469237: Only trace where BINDNAME will choose the global object.

Revised per Igor's comments: recognize frames that might end up with an eval-accessible call object by testing the HEAVYWEIGHT flag.  Changed to use fp->fun instead of fp->caller.

Comment 93

[Jim Blandy :jimb]

Waiting to commit this; three oranges in TM tinderbox at the moment, failures from 89d209a23a3a.

Comment 94

[Andreas Gal :gal]

Just check in over the orange. The single json failure is being investigated.

Comment 95

[Brendan Eich [:brendan]]

(In reply to comment #93)
> Waiting to commit this; three oranges in TM tinderbox at the moment, failures
> from 89d209a23a3a.

Sorry, I was looking at a blocker and only jumped on the orange in the last half hour. Sayrer points out that when orange is well-understood and confined to a few tests implicating the changeset that seemed to break them, you can check in over top -- assuming the miscreant (me) is on the case.

Or back me out, that works too. Again, my apologies -- but Rob's right, orange is not one color, or should not be.

/be

Comment 96

[Andreas Gal :gal]

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

Comment 97

[Robert Sayre]

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

Comment 98

[Brendan Eich [:brendan]]

Comment on attachment 373955 [details] [diff] [review]
Bug 469237: Only trace where BINDNAME will choose the global object.

I drive-by-review a lot but missed this patch till now.

>+    JSStackFrame *fp = cx->fp;
>+    JSObject *scope;

Nit: canonical name is obj, scope is for JSScope *.

>+
>+    if (fp->fun) {
>+        // We can't trace BINDNAME in functions that contain direct
>+        // calls to eval, as they might add bindings which
>+        // previously-traced references would have to see.
>+        if (JSFUN_HEAVYWEIGHT_TEST(fp->fun->flags))
>+            ABORT_TRACE("Can't trace JSOP_BINDNAME in heavyweight functions.");
>+
>+        // In non-heavyweight functions, we can safely skip the call
>+        // object, if any.
>+        scope = OBJ_GET_PARENT(cx, FUN_OBJECT(fp->fun));

This is the wrong function object parent -- the right funobj to use here is fp->callee, not FUN_OBJECT(fp->fun). Testcase:

var g = 0;
function f(n) {
    for (let i = 0; i < n; i++)
        g = g + i;
}

// This traces and produces correct results.
f(10);
print(g);

// Set up another global object.
var t = evalcx('this');
t.g = 0;
t.f = clone(f, t);

// This should trace, but aborts in TraceRecorder::record_JSOP_BINDNAME.
evalcx('f(5)', t);
print(t.g);

Need a followup bug, should block 1.9.1 but not b4.

/be

Comment 99

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/79f061c3eee7

Comment 100

[Bob Clary [:bc] (inactive)]

js/src/trace-test.js

Comment 101

[Bob Clary [:bc] (inactive)]

(In reply to comment #100)
not yet in js/src/trace-test.js

Comment 102

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.