Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 482007 [details] [diff] [review]
patch

This patch changes when debug hooks, probes, and PutActivationObjects() get called. The callee is responsible for it all, and we ensure that frame hooks/probes get called after constructing |this|. Frame prologue/epilogue stuff has been moved out of Invoke and method JIT stubs, and into:
 * Top and bottom of Interpret().
 * In inline_call and inline_return of Interpret().
 * Prologue and epilogue of method JIT'd functions.

In the process this patch also rewrites the tracer integration. It became a lot more commented and robust. The tricky part, historically, was figuring out what to do with JSOP_RETURN, and there were poorly-thought hacks in place to pass tests. Now there is an explanation of the problem and a frame flag that should solve it cleanly.

This patch also fixes some error handling, balancing bugs with hooks/probes, and fixes a bug where the DeclEnv of a generator didn't get the right frame.
Attachment #482007 - Flags: review?(lw)
Comment on attachment 482007 [details] [diff] [review]
patch

This looks really nice to me. I can almost fool myself into thinking I understand what's going on now. Some comments:

- why add jsprobes.h everywhere?

- I have a build failure in jsinterpinlines.h: JSStackFrame::resetInvokeCallFrame() uses the old JSFRAME_BAILED_AT_RETURN flag. Is that just me? (That file is included in the patch, so it's not just missing.)

>@@ -177,8 +175,8 @@ top:
>  * either because of a call into an un-JITable script, or because the call is
>  * throwing an exception.
>  */
>-static bool
>-InlineReturn(VMFrame &f, JSBool ok, JSBool popFrame)
>+static void
>+InlineReturn(VMFrame &f)
> {
>     JSContext *cx = f.cx;
>     JSStackFrame *fp = f.regs.fp;

There's not enough context to see it, but the comment before InlineReturn is no longer valid. (It refers to the removed parameters.)
(Assignee)

Comment 2

7 years ago
Thanks, Steve. It probably needs to rebased, I'll update to tip, and also fix the InlineReturn comment.

I added jsprobes.h because jsinterpinlines.h started using the probe hook. Is there something better I can do?
(Assignee)

Comment 3

7 years ago
Created attachment 482345 [details] [diff] [review]
part 2: fix disabled ICs

Follow-up to fix a bug exposed in --disable-polyic, and also in the browser embedding. JSOP_BEGIN's removal means we can't easily use the property cache anymore.

This patch adds a "usePropCache" bit to ICs and refactors a bit to make it work.
Attachment #482345 - Flags: review?(dmandelin)
But why do you need to include jsprobes.h in all of the .cpp files? It's
already included in jsinterpinlines.h.

Oh! Do we flatten out our include lists in .cpp's?
(Assignee)

Comment 5

7 years ago
It doesn't look like it's included in jsinterpinlines.h
Attachment #482345 - Flags: review?(dmandelin) → review+
Sorry, just confusing myself. I imported on top of my patch stack, and it's included in mine. I'll qpop -a.

So change my comment to "why not include in jsinterpinlines.h instead of all the .cpp files?"
(In reply to comment #6)
> Sorry, just confusing myself. I imported on top of my patch stack, and it's
> included in mine. I'll qpop -a.
> 
> So change my comment to "why not include in jsinterpinlines.h instead of all
> the .cpp files?"

It's a fair question. But does jsinterpinlines.h itself depend on jsprobes.h?

If yes, then the current approach is a bug and jsinterpinlines.h should satisfy its own dependency, and then any .cpp files that directly depend on jsprobes.h should do likewise. No bootlegging via an -inlines.h/-inl.h file.

If no, and the .cpp files modified by the patch to include jsprobes.h all have direct dependencies on jsprobes.h, then never mind!

/be
js/jsd/jsd_scpt.c is using JS_FirstValidPC (in jsd_GetFirstValidPC).

I'm trying this out to see if I get matching enters/exits, and so far it's been perfect except when it seg faults.

Unfortunately, it usually segfaults. This is running the full browser. The seg faults happen at different times in different code each time, always looking like memory corruption (usually JS memory, but not always.)

The crashes are probably resulting from my probe code, but I'm not sure, because it crashes even when the probes don't do anything. (cx->functionCallback is NULL.)

I am compiling with --enable-trace-jscalls and --enable-functiontimer, but I am not setting MOZ_FT (so --enable-functiontimer shouldn't do anything) and I am setting NSPR_LOG_MODULES=JSDiagnostics:4 (so --enable-trace-jscalls shouldn't be doing much, since cx->functionCallback is NULL. It is only non-NULL if you set NSPR_LOG_MODULES=JSDiagnostics:5)

I only get a crash if I set NSPR_LOG_MODULES=JSDiagnostics:4, though, so it's possible that this has nothing to do with the probes and only requires your patch plus JSDiagnostics output. I'm going to recompile without any special --enable flags and see what happens.

My test procedure, btw, is to start up minefield, go to about:config, then go to <http://people.mozilla.org/~sfink/slowcalls-proto/slowcalls.html>.

Comment 9

7 years ago
Comment on attachment 482007 [details] [diff] [review]
patch

Great patch!  My internal model-checker exploded, but, for what its worth, I can't find any real problems.  Only small requests:

>+    /* Don't call the script prologue if executing between Method and Trace JIT. */
>+    if (!(interpFlags & (JSINTERP_RECORD | JSINTERP_SAFEPOINT))) {
>+        if (!ScriptPrologue(cx, regs.fp))
>+            goto error;
>+    }

Could you add a JS_ASSERT(regs.pc == script->code) in the then-branch?

Another question: when I first read this (and some of the related code), I was wondering whether these flags were independent variations (like JSStackFrame::flags or JSFunction::flags), but it seems that there are three mutually-exclusive modes: record, safepoint, "normal".  To help readers, do you suppose you could use a tri-value "interpMode" and use mode queries against JSINTERP_NORMAL, 

>+        /* The JIT inlines ScriptEpilogue. */
>+  jit_return:
>+        CHECK_INTERRUPT_HANDLER();

This is a bit confusing without seeing 'goto jit_return', so could the CHECK_INTERRUPT_HANDLER just go right before 'goto jit_return' ?

>     JS_ASSERT(cx->regs == &regs);
>     *prevContextRegs = regs;
>     cx->setCurrentRegs(prevContextRegs);
>-    return interpReturnOK;
>+    goto leave_interpret;
> }

This is pre-existing, but could we have the regs-restoration only once, after leave_interpret, instead of before on both incoming paths?  In fact, in place of the MUST_FLOW_THROUGH("leave_interpret"), could we just have a guard:

struct InterpExitGuard {
  JSFrameRegs *prevContextRegs;
  InterpExitGuard(JSFrameRegs &regs) {
    prevContextRegs = cx->regs;
    cx->setCurrentRegs(&regs);
    ++cx->interpLevel;
  }
  ~InterpExitGuard() {
    --cx->interpLevel;
    *prevContextRegs = *cx->regs;
    cx->setCurrentRegs(prevContextRegs);
  }
} interpGuard(regs);

(Note, with local classes, this can be declared in-place in js::Interpret.)
With this, I think there could be two returns, one for "normal" exits and one for "exit on safe point", which could be commented as such.

>+    if (fp->isFunctionFrame() && !fp->isEvalFrame() && !fp->isYielding())
>+        PutActivationObjects(cx, fp);

Its great to see logic this in one place, rather than implicitly in three places.  Since you have the 'why' in your head, perhaps you could put a comment explaining why we don't put activation objects for eval/yielding frames... maybe "An eval frame's parent owns its activations objects.  A yielding frame's activation objects are transferred to the floating frame stored in the generator."

>+        // For consistency with Interpret(), always run the script epilogue.
>+        // This simplifies interactions with RunTracer().

First sentence is reassuring.  The second leaves me asking "how?"... perhaps either explain how or remove the second sentence?

> static inline JSBool
> PartialInterpret(VMFrame &f)
> {
>     JSContext *cx = f.cx;
>     JSStackFrame *fp = cx->fp();
> 
>+

Extra \n

>+static bool
>+HandleFinishedFrame(VMFrame &f, JSStackFrame *entryFrame)

Fantastic comments

>+     * Since the only scenario where this fixup is NOT needed is a normal exit
>+     * from the interpreter, we can cleanly check for this scenario by checking
>+     * a bit it sets in the frame.

Given this meaning of leftInterpreter, would it be too verbose to name the flag leftInterpreterNormally?

>+ * This function can leave any number of new additional frames after
>+ * returning.
>+ */
>+static bool
>+EvaluateExcessFrame(VMFrame &f, JSStackFrame *entryFrame)

Maybe you could be a bit more explicit:
"While this function is guaranteed to make progress, but it may not actually finish/pop the current frame."
?

>     /* IMacros are guaranteed to have been removed by now. */
>     JS_ASSERT(!entryFrame->hasImacropc());

Could you also assert f.fp() == entryFrame?

Random request while reading the code in RunTracer: could you add JS_ASSERT(!cx->throwing) in the TA_ERROR case after MonitorTracePoint?

Lastly: it seemed like there was some commonality between ScriptPrologue/ScriptEpilogue and stubs::EnterScript/stubs::LeaveScript that could be factored into one place.
Attachment #482007 - Flags: review?(lw) → review+
(In reply to comment #7)
> (In reply to comment #6)
> > So change my comment to "why not include in jsinterpinlines.h instead of all
> > the .cpp files?"
> 
> It's a fair question. But does jsinterpinlines.h itself depend on jsprobes.h?

Yes, it does. And the .cpp files that use jsprobes.h themselves already include it.

My no-option build finished, and does not crash. I retried with only --enable-functiontimer. No crash. I expect it'll be --enable-trace-jscalls; will try valgrind when I have time. I'm also hearing other people talking about intermittent crashes, so perhaps none of this matters.
I'm full o' feces. The functionCallback is non-NULL at :4 or higher, not :5 or higher.

I'm only getting crashes if the probes are firing and doing stuff. Since they were mismatched before, they're going from broken one way to broken another way, and shouldn't block this patch.
(Assignee)

Comment 12

7 years ago
(In reply to comment #9)
> and use mode queries against JSINTERP_NORMAL, 

Did this get cut off?

> Could you add a JS_ASSERT(regs.pc == script->code) in the then-branch?

Sure; had to modify it for generators though.

> would it be too verbose to name the flag leftInterpreterNormally?

Maybe "FINISHED_IN_INTERPRETER" is more clear for the verbosity.
(Assignee)

Comment 13

7 years ago
Took care of the other comments, also removed bad spreading of #include jsprobes.h.
(Assignee)

Comment 14

7 years ago
> Random request while reading the code in RunTracer: could you add
> JS_ASSERT(!cx->throwing) in the TA_ERROR case after MonitorTracePoint?

This is a great catch, that I accidentally convinced you was covered by the assert. Deep bails, our favorite feature, trigger the assert. Got a test case, will include with a new patch.

Comment 15

7 years ago
> > and use mode queries against JSINTERP_NORMAL, 

Hm, I guess I meant to put '?' instead of ','
(Assignee)

Comment 16

7 years ago
Created attachment 483004 [details] [diff] [review]
v2

combined patch to checkin. includes test case for when a trace throws an error inside a builtin, where we'd accidentally skip a try block in the newest frame.
Attachment #482007 - Attachment is obsolete: true
Attachment #482345 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #483004 - Flags: review+
I think this will fix the following seven jstests, which I guess have been broken since JSOP_BEGIN landed:

REGRESSIONS
    ecma\extensions\trapflatclosure.js
    e4x\decompilation\regress-429249.js
    ecma_3\extensions\regress-429248.js
    js1_5\extensions\regress-422137.js
    js1_5\extensions\regress-429264.js
    js1_5\extensions\regress-431428.js
    js1_7\decompilation\regress-429252.js
FAIL
Latest version of the patch still removes JS_FirstValidPC, which breaks js/jsd/jsd_scpt.c in jsd_GetFirstValidPC. I'm doing


diff --git a/js/jsd/jsd_scpt.c b/js/jsd/jsd_scpt.c
--- a/js/jsd/jsd_scpt.c
+++ b/js/jsd/jsd_scpt.c
@@ -507,7 +507,7 @@ jsd_GetClosestPC(JSDContext* jsdc, JSDSc
 jsuword
 jsd_GetFirstValidPC(JSDContext* jsdc, JSDScript* jsdscript)
 {
-    return (jsuword) JS_FirstValidPC(jsdc->dumbContext, jsdscript->script );
+    return (jsuword) jsdscript->script;
 }
 
 jsuword
(Assignee)

Comment 19

7 years ago
Should fixed now, I backed out the patch which added FirstValidPC.
Warning: with the JSOP_BEGIN removal patch applied, I get memory corruption in a full firefox build if I emit a native call in record_JSOP_STOP and record_JSOP_RETURN. I have it calling a dummy function that just returns true, and it still crashes in random places with memory corruption.

I'm trying to put together a minimal test case right now, but I have to do a full browser rebuild for it.

I can file this as a separate bug if you want to leave this as a "then don't do that!" for now.
(Assignee)

Comment 21

7 years ago
I just tried landing this and it didn't stick, so will backout shortly. One important item was a 7% v8 regression, entirely in deltablue. The major win from the new-IC patch, apparently, was *disabling* empty scripts, allowing them to JIT.

This patch re-enabled empty scripts, so a lot in this benchmark stopped JITing.

To get this patch landed again it'll be faster to temporarily disable empty scripts, after that I would like to move empty scripts to be per-compartment so we can JIT them.
(Assignee)

Comment 22

7 years ago
Created attachment 484117 [details] [diff] [review]
patch to checkin

Combined patch to checkin. Includes backout of FirstValidPC patch, and some test case fixes (there was a case where we'd call both Execute and Call hooks).

Steve, there is still, I think, a double-probe call here. js::Execute calls an "execute" probe, and the interpreter & JIT will then also call the "callJSFun" probe. If that's a problem, it can be fixed by sniffing for isExecuteFrame() like the call hooks do.
Just a note: I see JSOP_UNUSED180 in jsopcode.tbl, so in case anyone has missed it, the comment at the top of that file talks about reclaiming such unused ops before adding a new one (like JSOP_BEGIN) at the bottom.

/be
Blocks: 605330
(Assignee)

Comment 24

7 years ago
take two: http://hg.mozilla.org/tracemonkey/rev/e000b5963fde
Whiteboard: fixed-in-tracemonkey

Comment 25

7 years ago
http://hg.mozilla.org/mozilla-central/rev/e000b5963fde
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 606662
Depends on: 620407
(Assignee)

Updated

7 years ago
No longer depends on: 620407
You need to log in before you can comment on or make changes to this bug.