680562 - Remove CHECK_INTERRUPT_HANDLER

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER with jimb's new mechanism for enabling interrupts.

Comment 1

[Luke Wagner [:luke]]

Awesome, these are way gross.

Comment 2

[Luke Wagner [:luke]]

I just noticed this is unblocked.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm

Comment 4

[Jim Blandy :jimb]

OMG (In reply to Jason Orendorff [:jorendorff] from comment #0)
> Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER
> with jimb's new mechanism for enabling interrupts.

???? Oh, that. :(

Comment 5

[Jim Blandy :jimb]

Shouldn't there be a change to JS_SetInterrupt, JS_ClearInterrupt, to actually use the InterpreterFrames chain, as JSScript::initScriptCounts and JSScript::ensureHasDebugScript do?

Comment 6

[Jim Blandy :jimb]

Comment on attachment 640040 [details] [diff] [review]
rm

Review of attachment 640040 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ -917,5 @@
>  template<typename T>
>  class GenericInterruptEnabler : public InterpreterFrames::InterruptEnablerBase {
>    public:
>      GenericInterruptEnabler(T *variable, T value) : variable(variable), value(value) { }
> -    void enableInterrupts() const { *variable = value; }

"There's no need to restate the type name in member function names." Thanks!

::: js/src/jsinterp.h
@@ +241,5 @@
>      ~InterpreterFrames();
>  
>      /* If this js::Interpret frame is running |script|, enable interrupts. */
>      inline void enableInterruptsIfRunning(JSScript *script);
> +    inline void enableInterruptsUnconditionally() { enabler.enable(); }

This has no callers, but it seems like it needs to be used by JS_SetInterrupt.

Comment 7

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #5)

You're right, but it's weird, because I remember writing that code; I guess I lost it somehow...

> This has no callers, but it seems like it needs to be used by JS_SetInterrupt.

Yeah, that is what I added for JS_SetInterrupt :P

Comment 8

[Luke Wagner [:luke]]

Attachment: rm

qref'd 'n everything

On a related note: has it every happened to you that you have some un-qref-d changes, then you push an empty patch for the try-server message, push to try, qpop, qdel the try patch, and then notice you've lost your changes?

Comment 9

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #8)
> On a related note: has it every happened to you that you have some un-qref-d
> changes, then you push an empty patch for the try-server message, push to
> try, qpop, qdel the try patch, and then notice you've lost your changes?

Well, if you have uncommitted changes and qnew (to add the try chooser text), then that qnew will gather up the uncommitted changes into the new patch. And then you pop and qdel that. Possible?

Comment 10

[Jim Blandy :jimb]

VC tools should never lose your work; qdel should totally support "undo". But I think it's right for qnew to gather up uncommitted changes.

Comment 11

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #9)
Yes, that's exactly what happens.  I've had to retrieve the patch from try server before :)

Comment 12

[Jim Blandy :jimb]

Comment on attachment 642818 [details] [diff] [review]
rm

Review of attachment 642818 [details] [diff] [review]:
-----------------------------------------------------------------

Okay --- that makes more sense!

Comment 13

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e406f71fadb

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e406f71fadb