570652 - JM: Operation callback for methodjit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Drake [:adrake]]

Attachment: Patch

Adds a generalized mechanism for interrupting code flow, and allows the operation callback to work within the methodjit.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 449784 [details] [diff] [review]
Patch


>+JSBool
>+js_ExecutionInterrupted(JSContext *cx)

Nit: This name makes the return value confusing - how about js_InterruptExecution? Hrm. I don't have any better ideas so up to you :)

>+    JSBool result = JS_TRUE;
>+    if (cx->interruptFlags & JS_INTERRUPT_OPERATION_CALLBACK) {
>+        result = js_InvokeOperationCallback(cx) && result;
>+    }

Nit: If condition and statement are one line, house style omits braces.

>-    volatile jsint      operationCallbackFlag;
>+#define JS_INTERRUPT_OPERATION_CALLBACK 0x1
>+    volatile jsword     interruptFlags;

I think you want to keep this a jsint, PR_AtomicSet() takes a 32-bit integer and |jsword| is pointer-sized. Also, I think we're moving in favor of "static const" over C macros.

>+            if (analysis[PC].nincoming > 0) {
>+                RegisterID cxreg = frame.allocReg();
>+                masm.load32(FrameAddress(offsetof(VMFrame, cx)), cxreg);

This must be loadPtr()

>+                prepareStubCall();
>+                stubCall(stubs::Interrupt, Uses(0), Defs(0));
>+
>+                jump.linkTo(masm.label(), &masm);


Let's put this in the slow path emitter instead, to help fast-path icache. Examples are in nunbox/FastOps.cpp, will look something like:

    stubcc.linkExit(jump);
    stubcc.leave();
    stubcc.call(stubs::Interrupt);
    stubcc.rejoin(0);

Thanks for taking this, looking forward to new patch!

Comment 2

[Andrew Drake [:adrake]]

Fixed everything but the jsword thing. Per:

-    JS_ATOMIC_SET(&cx->operationCallbackFlag, 1);
+    JS_ATOMIC_SET_MASK(const_cast<jsword*>(&cx->interruptFlags),
+                       JS_INTERRUPT_OPERATION_CALLBACK);

PR_AtomicSet is no longer being called, and js_AtomicSetMask (for better or for worse) takes a jsword.

I've attached the patch, but here's some perf numbers (average of 3 trace-test.py)

No patch: ~54s
With original patch: ~55s
With this patch: ~60s  

It looks like the stubcc stuff slowed it down ~8%.

Comment 3

[Andrew Drake [:adrake]]

Attachment: Patch r0->r1

Comment 4

[Andrew Drake [:adrake]]

Attachment: Patch r1

Comment 5

[Igor Bukanov]

Comment on attachment 449794 [details] [diff] [review]
Patch r0->r1

> JS_PUBLIC_API(void)
>diff -r ebc29d698477 js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp	Mon Jun 07 21:27:37 2010 -0700
>+++ b/js/src/jscntxt.cpp	Mon Jun 07 22:01:45 2010 -0700
>@@ -2231,14 +2231,14 @@
> JSBool
> js_InvokeOperationCallback(JSContext *cx)
> {
>-    JS_ASSERT(cx->interruptFlags & JS_INTERRUPT_OPERATION_CALLBACK);
>+    JS_ASSERT(cx->interruptFlags & JSContext::INTERRUPT_OPERATION_CALLBACK);
> 
>     /*
>      * Reset the callback flag first, then yield. If another thread is racing
>      * us here we will accumulate another callback request which will be
>      * serviced at the next opportunity.
>      */
>-    cx->interruptFlags &= ~JS_INTERRUPT_OPERATION_CALLBACK;
>+    cx->interruptFlags &= JSContext::INTERRUPT_OPERATION_CALLBACK;

This is non-atomic operation. As such if another thread sets another flag in cx->interruptFlags at the moment of cx->interruptFlags &= that flag could be cleared. I suppose we need JS_ATOMIC_CLEAR_MASK here.

Comment 6

[Andrew Drake [:adrake]]

Good catch, thanks! Patch attached.

Comment 7

[Andrew Drake [:adrake]]

Attachment: Patch r2

Comment 8

[Andrew Drake [:adrake]]

Attachment: Patch r1->r2

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 449803 [details] [diff] [review]
Patch r2

r=me based on interdiff - "Patch r2" seems to have extra stuff at the top for some reason, so I looked at the bottom half.

Sunspider was 0.6% slower with inline Interrupt call and 0.5% slower with out-of-line Interrupt call. That seems like noise to me - it's hard to measure this right now, since we don't compile a lot, and trace-tests will get you a lot more than js code. Still, if the callback only costs us <1% on SunSpider so far, that's great.

Comment 10

[Andrew Drake [:adrake]]

Alright, I may have screwed up the export command for the cumulative one; I maintained this as a series in a queue. I folded it up, merged, and pushed.

Comment 11

[Igor Bukanov]

Comment on attachment 449805 [details] [diff] [review]
Patch r1->r2

>+void
>+js_AtomicUnsetMask(jsword *w, jsword mask)
>+{
>+    jsword ov, nv;
>+
>+    do {
>+        ov = *w;
>+        nv = ov &= (~mask);
>+    } while (!js_CompareAndSwap(w, ov, nv));

This is wrong. The loop should look like (the same structure as in AtomicSetMask):
    do {
        ov = *w;
        nv = ov & ~mask;
    } while (!js_CompareAndSwap(w, ov, nv));

Also use Clear, not Unset, in the name of macros and functions do be consistent with the rest of code.

Comment 12

[Andrew Drake [:adrake]]

Thanks, I have no idea why I screwed that up. Did the rename as well and pushed the (tiny) fix.