Closed Bug 492918 Opened 15 years ago Closed 15 years ago

TM: trace aborts if function closes over a variable and then modifies it

Categories

(Core :: JavaScript Engine, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: dmandelin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

See attached testcases.  The first one is the one I initially attached, incorrectly, to bug 492914.  The second directly modifies the value.  Brendan says they're the same issue.
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2a1
Brendan, in Comment 84 of bug 460050 you mentioned that one patch might solve both problems (i.e., this bug).  Have you already started on that patch for this bug?  If we haven't started, I was thinking of aiming dmandelin at this issue as his current bug is seemingly a lower priority (bug 480621 3d-morph).  

Thoughts?
Assignee: brendan → dmandelin
Here's the design I'm about to try to code up: it seems that the LIR to access the upvar is the same as for "normal" upvars (ones that use JSOP_GETUPVAR). That code seems like it should work for any upvar access where the function containing the definition is on the stack at the time the upvar is accessed. And I believe this holds if we get a non-null frame inside activeCallOrGlobalSlot.

The main difference is that the static link count ('skip') and slot number are not encoded in the bytecode. But we can get the static link count by looking at the parent chain while recording. And property lookup should give us the slot number.

So the strategy will be to compute the parameters to js_GetUpVarOnTrace, use those to generate a call, and refactor as necessary.
Attached patch WIP (obsolete) — Splinter Review
This patch shouldn't work, but it seems to work on these test cases. The reason it shouldn't work is that for incName, I just generate the LIR to access the upvar, but no code to increment it or anything. Anyway, I think I know how to generate the increment code, but the general problem is more difficult. See next comment.
The WIP patch doesn't handle this case, because it uses SETNAME/BINDNAME, and I haven't touched BINDNAME yet. I suppose the same general idea might work there, but I have to better understand how those ops work and how they are traced.
Idea for BINDNAME/SETNAME with upvars: Currently BINDNAME aborts if the result is not the global object. We could make it also be OK with call objects. The result would be the call object. Currently SETNAME also aborts if the argument is not the global object. We could make it accept call objects and then use something like the other upvar machinery to make it trace.
fwiw, the WIP definitely doesn't work right on the original go game testcase (produces incorrect output).  Not sure why it works on the testcases here but not that one.
I just realized that this is a bunch more complicated than I thought. Consider this version of test case 2 (just so it uses setname):

  function k(f_arg) { [loop] { f_arg(); } }
  function t() {
    var x = 1;
    k(function () { x = 2; });
    print(x);
  }

At the point where the lambda is called, the stack looks like this:

  [0] script main
  [1] t             static link to [0 main]
  [2] l             static link to [0 main]

Now we need to create the activation record for the lambda, including of course the static link so it can access upvars. The lambda is nested inside t, so the static link should point to t. But there is no path from the caller's activation record to t, so there is no way we can reach the right record by following N static links, so there is no way to compile this with static links.

Another view that reaches the same conclusion is with the display. At this point, the display looks like:

  [static level 0] script main
  [static level 1] k

because k was the last function called with static level 1. Thus, t is not anywhere in the display, and so cannot be reached through the display.

Anyway, all that is just to say that this isn't an algol-style nested functions situation. (Compare this to the original simplified go test case, where a function was passed as an argument, but that function didn't use the upvar: the upvar was used in an algol-style way.) Unless I'm wrong and it is actually that simple, in which case let me know. Otherwise, I think this is going to have to look more like a standard property access, so I'll have to rethink the design a bit.
Downward funargs are definitely not Algol-like -- Pascal innovation, much later, IIRC. Sorry if I was unclear. Still, we are nesting in the activation of t -- look at the dynamic chain:

  [0] script main
  [1] t             static link to [0 main]
  [2] k             static link to [0 main]

It does reach t. The problem is that you can't pick the first matching static level, as with non-escaping Algol-like functions. You have to search for the right callee.

/be
Yes, I know that t is on the dynamic chain, which at least means it can be accessed by means of slots, although I don't think that's guaranteed to be true through upvar mutations via bindname/setname (incname stuff as well). I don't know if there are any criteria that the implementation can use to prove that the item is on stack. I also don't know how to find the right activation record even if it is on the stack. I need to study the existing SpiderMonkey stuff on that. 

This test case modifies the upvar in the second 'f' on stack, although if you change the call in f of q(g_arg) to q(g), it modifies the first, so simply searching the dynamic stack for a matching function can't be how it works:

function q(g_arg) {
  for (var i = 0; i < 5; ++i)
    g_arg();
}

function f(i, g_arg) {
  var upvar = i;

  function g() {
    upvar += 'x';
  }

  if (g_arg == undefined) {
    f('second_f_', g);
  } else {
    q(g_arg);
  }

  print(upvar);
}

f('first_f_');
Yeah, can't search for callee -- don't want to, anyway. Don't we have a scope chain pointing at a Call object with a frame pointer in its private slot? That is the key.

/be
Attached patch WIP 2Splinter Review
This version traces jsop_setname. It gets a 5x or so speedup on a single loop that sets an upvar. There's some stuff still in there for INCNAME that I haven't finished yet; ignore that part.

Unfortunately, it generally slows down the code when there is an outer loop that contains (dynamically) the call to the function defining the upvar. The reason is that the inner function has a different identity each time through the outer loop, so the outer loop exits and records a new trace. This generally causes us to hit max_branches and we stop tracing more loops, but we'll still enter the inner trace.

Another issue exposed by this patch is that we don't trace inner jsop_lambda. Thus, the outer loop will not trace, and we'll go on and off trace at each call to the inner trace.
Attachment #378703 - Attachment is obsolete: true
Depends on: 495329
Depends on: 495330
Depends on: 495331
No longer depends on: 495331
Depends on: 507449
I just tested, and all three testcases here trace completely.  Of course all the bugs this depends on are also fixed...

David, is this bug just fixed now? Seems to be to me.
Yes, I think we can consider this fixed now.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.