527147 - flat lambdas' parents differ between recording and trace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

In the following code:

function g() {
    var x = 1;
    var result = "abcd".replace('b', function(){return x});
}
for (var i = 0; i < 100; ++i) {
    g();
}

The parent of |function(){return x}| differs between when we are interpreting/recording and executing on trace.  This can be observed by putting a breakpoint in jsstr.cpp, FindReplaceLength, on the line:

        *sp++ = OBJECT_TO_JSVAL(OBJ_GET_PARENT(cx, lambda));

For the first three executions, the parent is the global object, for the fourth (on trace), the parent's class is a js_FunctionClass.  This causes a problem for tracing String.replace with a lambda since the trace type of a function differs from other objects.

The root of the discrepancy is in TraceRecorder::record_JSOP_LAMBDA_FC, which effectively passes cx->fp->argv[-2] (the caller object) to js_AllocFlatClosure as the parent.  In the interpreter, cx->fp->scopeChain is passed, which is the global object for the above code.

I read that scopeChain can be lazily initialized, so, perhaps in some situations the caller object would in fact be the parent of a new lambda.  Is this possible?  If so, then the generated code would be technically correct.

Even if this behavior is allowed, though, it would be nice (for avoiding an always-wasted recording) if the trace type of a function's parent did not change from record-time to trace execution-time.  Is there any simple efficient way to produce the same parent as when interpreting?

Comment 1

[David Mandelin [:dmandelin]]

I don't think we have a way of computing the scope chain on trace that covers all cases. Bug 495331 would make it possible (or at least expand the number of cases), but that was backed out due to a perf regression and I've been on CrashKill duty since then.

If you'd like to fix up 495331, that would be totally awesome. Otherwise, I think you just need to make sure the scope chain is the global object, and abort if not, and pass the correct scope chain. (Passing the callee is just wrong, AFAICT.) See record_JSOP_LAMBDA for an example of doing just that.

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
> Otherwise, I
> think you just need to make sure the scope chain is the global object, and
> abort if not, and pass the correct scope chain. (Passing the callee is just
> wrong, AFAICT.) See record_JSOP_LAMBDA for an example of doing just that.

That sounds much nicer :)  Is it sufficient to do a simple record-time test that |OBJ_GET_PARENT(cx, FUN_OBJECT(fun)) == globalObj|, just like record_JSOP_LAMBDA?

Comment 3

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> (In reply to comment #1)
> > Otherwise, I
> > think you just need to make sure the scope chain is the global object, and
> > abort if not, and pass the correct scope chain. (Passing the callee is just
> > wrong, AFAICT.) See record_JSOP_LAMBDA for an example of doing just that.
> 
> That sounds much nicer :)  Is it sufficient to do a simple record-time test
> that |OBJ_GET_PARENT(cx, FUN_OBJECT(fun)) == globalObj|, just like
> record_JSOP_LAMBDA?

I think so.

Comment 4

[Luke Wagner [:luke]]

Attachment: only use globalObj as lambda-fc parent

Like so?  Anyone else I should run this by?  Parents are confusing things.

Comment 5

[David Mandelin [:dmandelin]]

Comment on attachment 410910 [details] [diff] [review]
only use globalObj as lambda-fc parent

Looks good. Just test it extensively. :-)

Comment 6

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/970cd5f87f42

Comment 7

[Brendan Eich [:brendan]]

Talked to Luke, this is all good. The trickiness is in JSCompiler::newFunction, where we make compile-time function objects whose parent slots are either null, or the global object the compiler was told will be used as the scope chain for a single execution of the global code whose source it was given.

Runtime cloned (unjoined, closure) function objects may have deeper scope chains linked by non-global parents, and they never have null parents. But the fun here is a compiler-created function object, so its parent is either null or the one true global.

The patch aborts on other than the one true global that the trace monitor knows about, which is extra goodness (in case of bugs with multiple globals).

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/970cd5f87f42