Avoid imacros for JSOP_GETELEM and JSOP_CALLELEM

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

10 years ago
This patch also introduces some methods, TR::enterDeepBail, TR::leaveDeepBail, and TR::finishGetProp, that will be helpful for tracing getters.

The new trace-test is just for a bit more coverage. While I was working on this patch CALLELEM was broken for a while, and trace-test.js didn't notice.
Assignee

Comment 1

10 years ago
Posted patch v1 (obsolete) — Splinter Review
A couple questions:

First, the way I'm using monitorRecording to clean up after a GETPROP or other such opcode: is that safe? monitorRecording will get called unless the GETPROP (CALLPROP, GETLOCALPROP, etc.) is immediately followed by a loop edge, which I claim can't happen. Should I be doing something less precarious?

Second: Some existing code in TR::snapshot is like this:
>     /*
>-     * If we are currently executing a traceable native or we are attaching a
>-     * second trace to it, the value on top of the stack is a jsval. Make a
>-     * note of this in the typemap.
>-     */
>-    if (pendingTraceableNative && (pendingTraceableNative->flags &
>-                                   JSTN_UNBOX_AFTER))
>-        typemap[stackSlots - 1] = TT_JSVAL;

I have a question about this, completely independent of the present bug/patch.

I think "we are attaching a second trace to it" means, we emitted a call to a function that returns a jsval, followed by unbox_jsval, which then at runtime flunked the type guard. Now we're recording a second case, to patch that guard. The comment suggests that in this case, we'll set typemap[(top)] = TT_JSVAL. But how does pendingTraceableNative get to be nonnull here?
Assignee: general → jorendorff
Attachment #391916 - Flags: review?(gal)
Assignee

Updated

10 years ago
Blocks: 507683

Comment 2

10 years ago
We usually do a callback hook instead of using monitorRecording. Its probably a trade-off. monitorRecording doesn't need the hook in the general interpreter path, but making monitorRecording fatter slows the overall recorder down. Otherwise I think both should be fine. The whole state machine gymnastics necessary when we unbox so we can get a post-unbox hook is pretty lame, but probably unavoidable.

As discussed previously there are two cases where pendingTraceableNative might be null. 1) is when we take care of emitting the code in one path, i.e. toString on str which doesn't really do anything, so we don't need a post hook. The other case is once we record attached traces. There pendingTraceableNative is also NULL, since the trace attaches to a guard now and we have no memory of the native that previously failed. Otherwise the patch looks good. Its a great first step. It will probably be slow for setters and getters due to the uncached lookup, but one step at a time.
Assignee

Comment 3

10 years ago
Posted patch v2Splinter Review
(In reply to comment #2)
> We usually do a callback hook instead of using monitorRecording. Its probably a
> trade-off. monitorRecording doesn't need the hook in the general interpreter
> path, but making monitorRecording fatter slows the overall recorder down.
> Otherwise I think both should be fine.

Would you like me to change it? I thought it would be worse to add a hook after every GETPROP because we still spend more time in the interpreter not recording than recording.

> The whole state machine gymnastics
> necessary when we unbox so we can get a post-unbox hook is pretty lame, but
> probably unavoidable.

Yeah. Since we're stuck with it, it might be better to just be really explicit about it:

  TR::record_JSOP_CALL() {
      ...
      afterThisOp = &afterNativeCall;
      return JSRS_CONTINUE;
  }

  void
  TR::afterNativeCall() { ...unbox... }

We would still be stuck with pendingWhatever fields, but at least there's sort of a unified scheme for the gymnastics. The hook code could be next to the record code instead of scattered all over the place. We could get rid of all the interpreter hooks like record_NativeCallbackComplete, so the non-recording interpreter would save a branch. Instead of having several checks in monitorRecording, there would just be one:

  if (tr->afterThisOp) {
      (tr->*afterThisOp)();
      tr->afterThisOp = NULL;
  }

If you like, I can file a bug for this.
Attachment #391916 - Attachment is obsolete: true
Attachment #392137 - Flags: review?(gal)
Attachment #391916 - Flags: review?(gal)
Assignee

Comment 4

10 years ago
v2 fixes a bug in v1 where the snapshot for a certain guard (the builtinStatus guard) was capturing pre-GETELEM state when it should be post-GETELEM state.

It also aborts when GETELEM is used on the global object, since there's no point recording that (we would just deep-bail). We should get rid of all this LeaveTraceIfGlobalObject stuff someday. :-P

Comment 5

10 years ago
Yeah, the dynamic post-call hook would clean up a lot of stuff. I like it. Reviewing the patch.

Updated

10 years ago
Attachment #392137 - Flags: review?(gal) → review+

Comment 6

10 years ago
Comment on attachment 392137 [details] [diff] [review]
v2


Maybe grab regs into a local reference and then do regs.pc/regs.sp. Then we probably don't need *pc here either, just regs.pc.

>+            const JSCodeSpec& cs = js_CodeSpec[*pc];
>+            cx->fp->regs->pc += cs.length;
>+            cx->fp->regs->sp -= (cs.format & JOF_INVOKE) ? GET_ARGC(pc) + 2 : cs.nuses;
>+            cx->fp->regs->sp += cs.ndefs;
>             JS_ASSERT_IF(!cx->fp->imacpc,
>                          cx->fp->slots + cx->fp->script->nfixed +
>                          js_ReconstructStackDepth(cx, cx->fp->script, cx->fp->regs->pc) ==
>                          cx->fp->regs->sp);
> 

>+
>+            /*
>+             * The return value was not available when we reconstructed the
>+             * stack, but we have it now. Box it. (Some opcodes, like
>+             * JSOP_CALLELEM, produce two values, hence the loop.)
>+             */

Does CALLELEM produced 2 boxed values? How?

>+    enterDeepBailCall();

I really don't like the name but I have no better suggestion.

>+    if (js_CodeSpec[*cx->fp->regs->pc].format & JOF_CALLOP)

Maybe we should make a helper function to get format(current-pc). Only if you feel like it.
Assignee

Comment 7

10 years ago
(In reply to comment #6)
> >+
> >+            /*
> >+             * The return value was not available when we reconstructed the
> >+             * stack, but we have it now. Box it. (Some opcodes, like
> >+             * JSOP_CALLELEM, produce two values, hence the loop.)
> >+             */
> 
> Does CALLELEM produced 2 boxed values? How?

Well, CALLELEM has .nuses=2, .ndefs=2. In that sense it produces two values. Whether they are boxed or unboxed is sort of beside the point here, since we have the typemap and this code doesn't care if the entry is TT_JSVAL or something else.

I think I could make it work with just one call to NativeToValue, but it would be a brittle, tricky sort of "simplification". The big picture here is not "box the return value" but "manually step the interpreter state to the next op". In that light the loop makes more sense.

> >+    enterDeepBailCall();
> 
> I really don't like the name but I have no better suggestion.

Which part don't you like? Instead of enter/leave we can do pre/post, before/after, setup/cleanup, etc. Instead of DeepBailCall it could be FallibleCall, _FAIL_Call (ugh), BuiltinCall, etc. And I could throw in "emit". So maybe:

  emitFallibleCallSetup
  emitFallibleCallCleanup

Of course there are still problems here. The callee, not the call, is "fallible", and I don't mean "fallible" in the established sense of "return value is 0 on error or exception" but rather "this function might deep-bail". Maybe we should invent a word for that.

I'll push it with the crummy names, and will happily patch them later if you find names you like.

> >+    if (js_CodeSpec[*cx->fp->regs->pc].format & JOF_CALLOP)
> 
> Maybe we should make a helper function to get format(current-pc). Only if you
> feel like it.

Ehrmm, it touches too many lines for this patch, but I agree.
Assignee

Comment 8

10 years ago
Pushed: http://hg.mozilla.org/tracemonkey/rev/7646cebfe2bb

(In reply to comment #7)
> The big picture here is not "box
> the return value" but "manually step the interpreter state to the next op".

I changed some of the comments around this code to emphasize this.
Whiteboard: fixed-in-tracemonkey

Comment 9

10 years ago
http://hg.mozilla.org/mozilla-central/rev/7646cebfe2bb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: wanted1.9.2+

Comment 10

10 years ago
trace-test  testCallElem
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.