Bug 1458567 part 1 - Don't invoke interrupt callback and Debugger onStep hook for internal JS engine interrupts. r=luke
authorJan de Mooij <jdemooij@mozilla.com>
Thu, 03 May 2018 16:01:01 +0200
changeset 472940 693e9f7a2b5983c71e0e2de324335cc7ef990611
parent 472939 8f9fc394b2c407b1fe5ac352bbd7c97c511ac117
child 472941 1eb83ac9acee5653c4a8eb80e847a7b8391d8f41
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs1458567
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1458567 part 1 - Don't invoke interrupt callback and Debugger onStep hook for internal JS engine interrupts. r=luke
js/public/Utility.h
js/src/gc/Allocator.cpp
js/src/gc/GC.cpp
js/src/irregexp/NativeRegExpMacroAssembler.cpp
js/src/jit/BaselineCompiler.cpp
js/src/jit/CodeGenerator.cpp
js/src/jit/CompileWrappers.cpp
js/src/jit/CompileWrappers.h
js/src/jsapi.cpp
js/src/vm/HelperThreads.cpp
js/src/vm/JSContext-inl.h
js/src/vm/JSContext.cpp
js/src/vm/JSContext.h
js/src/vm/Runtime.cpp
--- a/js/public/Utility.h
+++ b/js/public/Utility.h
@@ -297,17 +297,17 @@ HadSimulatedInterrupt()
             ReportOverRecursed(cx);                                           \
             return false;                                                     \
         }                                                                     \
     } while (0)
 
 #  define JS_INTERRUPT_POSSIBLY_FAIL()                                        \
     do {                                                                      \
         if (MOZ_UNLIKELY(js::oom::ShouldFailWithInterrupt())) {               \
-            cx->interrupt_ = true;                                            \
+            cx->requestInterrupt(js::InterruptReason::CallbackUrgent);        \
             return cx->handleInterrupt();                                     \
         }                                                                     \
     } while (0)
 
 # else
 
 #  define JS_OOM_POSSIBLY_FAIL() do {} while(0)
 #  define JS_OOM_POSSIBLY_FAIL_BOOL() do {} while(0)
--- a/js/src/gc/Allocator.cpp
+++ b/js/src/gc/Allocator.cpp
@@ -309,17 +309,17 @@ GCRuntime::gcIfNeededAtAllocation(JSCont
 {
 #ifdef JS_GC_ZEAL
     if (needZealousGC())
         runDebugGC();
 #endif
 
     // Invoking the interrupt callback can fail and we can't usefully
     // handle that here. Just check in case we need to collect instead.
-    if (cx->hasPendingInterrupt())
+    if (cx->hasAnyPendingInterrupt())
         gcIfRequested();
 
     // If we have grown past our GC heap threshold while in the middle of
     // an incremental GC, we're growing faster than we're GCing, so stop
     // the world and do a full, non-incremental GC right now, if possible.
     if (isIncrementalGCInProgress() &&
         cx->zone()->usage.gcBytes() > cx->zone()->threshold.gcTriggerBytes())
     {
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -3280,36 +3280,30 @@ void
 GCRuntime::requestMajorGC(JS::gcreason::Reason reason)
 {
     MOZ_ASSERT(!CurrentThreadIsPerformingGC());
 
     if (majorGCRequested())
         return;
 
     majorGCTriggerReason = reason;
-
-    // There's no need to use RequestInterruptUrgent here. It's slower because
-    // it has to interrupt (looping) Ion code, but loops in Ion code that
-    // affect GC will have an explicit interrupt check.
-    rt->mainContextFromOwnThread()->requestInterrupt(JSContext::RequestInterruptCanWait);
+    rt->mainContextFromOwnThread()->requestInterrupt(InterruptReason::GC);
 }
 
 void
 Nursery::requestMinorGC(JS::gcreason::Reason reason) const
 {
     MOZ_ASSERT(CurrentThreadCanAccessRuntime(runtime()));
     MOZ_ASSERT(!CurrentThreadIsPerformingGC());
 
     if (minorGCRequested())
         return;
 
     minorGCTriggerReason_ = reason;
-
-    // See comment in requestMajorGC.
-    runtime()->mainContextFromOwnThread()->requestInterrupt(JSContext::RequestInterruptCanWait);
+    runtime()->mainContextFromOwnThread()->requestInterrupt(InterruptReason::GC);
 }
 
 bool
 GCRuntime::triggerGC(JS::gcreason::Reason reason)
 {
     /*
      * Don't trigger GCs if this is being called off the main thread from
      * onTooMuchMalloc().
--- a/js/src/irregexp/NativeRegExpMacroAssembler.cpp
+++ b/js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ -561,22 +561,24 @@ NativeRegExpMacroAssembler::AdvanceRegis
         masm.addPtr(Imm32(by), register_location(reg));
 }
 
 void
 NativeRegExpMacroAssembler::Backtrack()
 {
     JitSpew(SPEW_PREFIX "Backtrack");
 
-    // Check for an interrupt.
+    // Check for an urgent interrupt. We don't want to leave JIT code and enter
+    // the regex interpreter for non-urgent interrupts. Note that interruptBits_
+    // is a bitfield.
     Label noInterrupt;
-    masm.branch32(Assembler::Equal,
-                  AbsoluteAddress(cx->addressOfInterruptRegExpJit()),
-                  Imm32(0),
-                  &noInterrupt);
+    masm.branchTest32(Assembler::Zero,
+                      AbsoluteAddress(cx->addressOfInterruptBits()),
+                      Imm32(uint32_t(InterruptReason::CallbackUrgent)),
+                      &noInterrupt);
     masm.movePtr(ImmWord(RegExpRunStatus_Error), temp0);
     masm.jump(&exit_label_);
     masm.bind(&noInterrupt);
 
     // Pop code location from backtrack stack and jump to location.
     PopBacktrack(temp0);
     masm.jump(temp0);
 }
--- a/js/src/jit/BaselineCompiler.cpp
+++ b/js/src/jit/BaselineCompiler.cpp
@@ -696,17 +696,17 @@ static const VMFunction InterruptCheckIn
 
 bool
 BaselineCompiler::emitInterruptCheck()
 {
     frame.syncStack(0);
 
     Label done;
     masm.branch32(Assembler::Equal,
-                  AbsoluteAddress(cx->addressOfInterrupt()), Imm32(0),
+                  AbsoluteAddress(cx->addressOfInterruptBits()), Imm32(0),
                   &done);
 
     prepareVMCall();
     if (!callVM(InterruptCheckInfo))
         return false;
 
     masm.bind(&done);
     return true;
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -12867,17 +12867,17 @@ CodeGenerator::visitAssertRangeV(LAssert
     masm.bind(&done);
 }
 
 void
 CodeGenerator::visitInterruptCheck(LInterruptCheck* lir)
 {
     OutOfLineCode* ool = oolCallVM(InterruptCheckInfo, lir, ArgList(), StoreNothing());
 
-    const void* interruptAddr = gen->runtime->addressOfInterrupt();
+    const void* interruptAddr = gen->runtime->addressOfInterruptBits();
     masm.branch32(Assembler::NotEqual, AbsoluteAddress(interruptAddr), Imm32(0), ool->entry());
     masm.bind(ool->rejoin());
 }
 
 void
 CodeGenerator::visitWasmInterruptCheck(LWasmInterruptCheck* lir)
 {
     MOZ_ASSERT(gen->compilingWasm());
--- a/js/src/jit/CompileWrappers.cpp
+++ b/js/src/jit/CompileWrappers.cpp
@@ -109,19 +109,19 @@ CompileRuntime::mainContextPtr()
 
 const void*
 CompileRuntime::addressOfJitStackLimit()
 {
     return runtime()->mainContextFromAnyThread()->addressOfJitStackLimit();
 }
 
 const void*
-CompileRuntime::addressOfInterrupt()
+CompileRuntime::addressOfInterruptBits()
 {
-    return runtime()->mainContextFromAnyThread()->addressOfInterrupt();
+    return runtime()->mainContextFromAnyThread()->addressOfInterruptBits();
 }
 
 #ifdef DEBUG
 bool
 CompileRuntime::isInsideNursery(gc::Cell* cell)
 {
     return UninlinedIsInsideNursery(cell);
 }
--- a/js/src/jit/CompileWrappers.h
+++ b/js/src/jit/CompileWrappers.h
@@ -44,17 +44,17 @@ class CompileRuntime
     const PropertyName* emptyString();
     const StaticStrings& staticStrings();
     const Value& NaNValue();
     const Value& positiveInfinityValue();
     const WellKnownSymbols& wellKnownSymbols();
 
     const void* mainContextPtr();
     const void* addressOfJitStackLimit();
-    const void* addressOfInterrupt();
+    const void* addressOfInterruptBits();
 
 #ifdef DEBUG
     bool isInsideNursery(gc::Cell* cell);
 #endif
 
     // DOM callbacks must be threadsafe (and will hopefully be removed soon).
     const DOMCallbacks* DOMcallbacks();
 
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -5720,23 +5720,23 @@ JS_PUBLIC_API(void)
 JS::InitConsumeStreamCallback(JSContext* cx, ConsumeStreamCallback callback)
 {
     cx->runtime()->consumeStreamCallback = callback;
 }
 
 JS_PUBLIC_API(void)
 JS_RequestInterruptCallback(JSContext* cx)
 {
-    cx->requestInterrupt(JSContext::RequestInterruptUrgent);
+    cx->requestInterrupt(InterruptReason::CallbackUrgent);
 }
 
 JS_PUBLIC_API(void)
 JS_RequestInterruptCallbackCanWait(JSContext* cx)
 {
-    cx->requestInterrupt(JSContext::RequestInterruptCanWait);
+    cx->requestInterrupt(InterruptReason::CallbackCanWait);
 }
 
 JS::AutoSetAsyncStackForNewCalls::AutoSetAsyncStackForNewCalls(
   JSContext* cx, HandleObject stack, const char* asyncCause,
   JS::AutoSetAsyncStackForNewCalls::AsyncCallKind kind)
   : cx(cx),
     oldAsyncStack(cx, cx->asyncStackForNewActivations()),
     oldAsyncCause(cx->asyncCauseForNewActivations),
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -1911,25 +1911,23 @@ HelperThread::handleIonWorkload(AutoLock
                              jit::CompileCompartment::get(builder->script()->compartment()),
                              &builder->alloc());
         builder->setBackgroundCodegen(jit::CompileBackEnd(builder));
     }
 
     FinishOffThreadIonCompile(builder, locked);
 
     // Ping the main thread so that the compiled code can be incorporated at the
-    // next interrupt callback. Don't interrupt Ion code for this, as this
-    // incorporation can be delayed indefinitely without affecting performance
-    // as long as the main thread is actually executing Ion code.
+    // next interrupt callback.
     //
     // This must happen before the current task is reset. DestroyContext
     // cancels in progress Ion compilations before destroying its target
     // context, and after we reset the current task we are no longer considered
     // to be Ion compiling.
-    rt->mainContextFromAnyThread()->requestInterrupt(JSContext::RequestInterruptCanWait);
+    rt->mainContextFromAnyThread()->requestInterrupt(InterruptReason::AttachIonCompilations);
 
     currentTask.reset();
 
     // Notify the main thread in case it is waiting for the compilation to finish.
     HelperThreadState().notifyAll(GlobalHelperThreadState::CONSUMER, locked);
 }
 
 void
--- a/js/src/vm/JSContext-inl.h
+++ b/js/src/vm/JSContext-inl.h
@@ -387,17 +387,17 @@ CallJSDeletePropertyOp(JSContext* cx, JS
 }
 
 MOZ_ALWAYS_INLINE bool
 CheckForInterrupt(JSContext* cx)
 {
     MOZ_ASSERT(!cx->isExceptionPending());
     // Add an inline fast-path since we have to check for interrupts in some hot
     // C++ loops of library builtins.
-    if (MOZ_UNLIKELY(cx->hasPendingInterrupt()))
+    if (MOZ_UNLIKELY(cx->hasAnyPendingInterrupt()))
         return cx->handleInterrupt();
 
     JS_INTERRUPT_POSSIBLY_FAIL();
 
     return true;
 }
 
 }  /* namespace js */
--- a/js/src/vm/JSContext.cpp
+++ b/js/src/vm/JSContext.cpp
@@ -1281,18 +1281,17 @@ JSContext::JSContext(JSRuntime* runtime,
     generatingError(false),
     cycleDetectorVector_(this),
     data(nullptr),
     outstandingRequests(0),
     jitIsBroken(false),
     asyncCauseForNewActivations(nullptr),
     asyncCallIsExplicit(false),
     interruptCallbackDisabled(false),
-    interrupt_(false),
-    interruptRegExpJit_(false),
+    interruptBits_(0),
     osrTempData_(nullptr),
     ionReturnOverride_(MagicValue(JS_ARG_POISON)),
     jitStackLimit(UINTPTR_MAX),
     jitStackLimitNoInterrupt(UINTPTR_MAX),
     getIncumbentGlobalCallback(nullptr),
     enqueuePromiseJobCallback(nullptr),
     enqueuePromiseJobCallbackData(nullptr),
     jobQueue(nullptr),
--- a/js/src/vm/JSContext.h
+++ b/js/src/vm/JSContext.h
@@ -87,16 +87,24 @@ enum class ContextKind
     HelperThread
 };
 
 #ifdef DEBUG
 bool
 CurrentThreadIsParseThread();
 #endif
 
+enum class InterruptReason : uint32_t
+{
+    GC = 1 << 0,
+    AttachIonCompilations = 1 << 1,
+    CallbackUrgent = 1 << 2,
+    CallbackCanWait = 1 << 3,
+};
+
 } /* namespace js */
 
 /*
  * A JSContext encapsulates the thread local state used when using the JS
  * runtime.
  */
 struct JSContext : public JS::RootingContext,
                    public js::MallocProvider<JSContext>
@@ -782,57 +790,52 @@ struct JSContext : public JS::RootingCon
 
   private:
     js::ThreadData<InterruptCallbackVector> interruptCallbacks_;
   public:
     InterruptCallbackVector& interruptCallbacks() { return interruptCallbacks_.ref(); }
 
     js::ThreadData<bool> interruptCallbackDisabled;
 
-    mozilla::Atomic<uint32_t, mozilla::Relaxed> interrupt_;
-    mozilla::Atomic<uint32_t, mozilla::Relaxed> interruptRegExpJit_;
-
-    enum InterruptMode {
-        RequestInterruptUrgent,
-        RequestInterruptCanWait
-    };
+    // Bitfield storing InterruptReason values.
+    mozilla::Atomic<uint32_t, mozilla::Relaxed> interruptBits_;
 
     // Any thread can call requestInterrupt() to request that this thread
-    // stop running and call the interrupt callback (allowing the interrupt
-    // callback to halt execution). To stop this thread, requestInterrupt
-    // sets two fields: interrupt_ (set to true) and jitStackLimit_ (set to
+    // stop running. To stop this thread, requestInterrupt sets two fields:
+    // interruptBits_ (a bitset of InterruptReasons) and jitStackLimit_ (set to
     // UINTPTR_MAX). The JS engine must continually poll one of these fields
-    // and call handleInterrupt if either field has the interrupt value. (The
-    // point of setting jitStackLimit_ to UINTPTR_MAX is that JIT code already
-    // needs to guard on jitStackLimit_ in every function prologue to avoid
-    // stack overflow, so we avoid a second branch on interrupt_ by setting
-    // jitStackLimit_ to a value that is guaranteed to fail the guard.)
+    // and call handleInterrupt if either field has the interrupt value.
     //
-    // Note that the writes to interrupt_ and jitStackLimit_ use a Relaxed
+    // The point of setting jitStackLimit_ to UINTPTR_MAX is that JIT code
+    // already needs to guard on jitStackLimit_ in every function prologue to
+    // avoid stack overflow, so we avoid a second branch on interruptBits_ by
+    // setting jitStackLimit_ to a value that is guaranteed to fail the guard.)
+    //
+    // Note that the writes to interruptBits_ and jitStackLimit_ use a Relaxed
     // Atomic so, while the writes are guaranteed to eventually be visible to
     // this thread, it can happen in any order. handleInterrupt calls the
     // interrupt callback if either is set, so it really doesn't matter as long
     // as the JS engine is continually polling at least one field. In corner
     // cases, this relaxed ordering could lead to an interrupt handler being
     // called twice in succession after a single requestInterrupt call, but
     // that's fine.
-    void requestInterrupt(InterruptMode mode);
+    void requestInterrupt(js::InterruptReason reason);
     bool handleInterrupt();
 
-    MOZ_ALWAYS_INLINE bool hasPendingInterrupt() const {
-        static_assert(sizeof(interrupt_) == sizeof(uint32_t), "Assumed by JIT callers");
-        return interrupt_;
+    MOZ_ALWAYS_INLINE bool hasAnyPendingInterrupt() const {
+        static_assert(sizeof(interruptBits_) == sizeof(uint32_t), "Assumed by JIT callers");
+        return interruptBits_ != 0;
+    }
+    bool hasPendingInterrupt(js::InterruptReason reason) const {
+        return interruptBits_ & uint32_t(reason);
     }
 
   public:
-    void* addressOfInterrupt() {
-        return &interrupt_;
-    }
-    void* addressOfInterruptRegExpJit() {
-        return &interruptRegExpJit_;
+    void* addressOfInterruptBits() {
+        return &interruptBits_;
     }
     void* addressOfJitStackLimit() {
         return &jitStackLimit;
     }
     void* addressOfJitStackLimitNoInterrupt() {
         return &jitStackLimitNoInterrupt;
     }
 
--- a/js/src/vm/Runtime.cpp
+++ b/js/src/vm/Runtime.cpp
@@ -426,27 +426,31 @@ JSRuntime::addSizeOfIncludingThis(mozill
         for (auto builder : jitRuntime_->ionLazyLinkList(this))
             rtSizes->jitLazyLink += builder->sizeOfExcludingThis(mallocSizeOf);
     }
 
     rtSizes->wasmRuntime += wasmInstances.lock()->sizeOfExcludingThis(mallocSizeOf);
 }
 
 static bool
-InvokeInterruptCallback(JSContext* cx)
+HandleInterrupt(JSContext* cx, bool invokeCallback)
 {
     MOZ_ASSERT(cx->requestDepth >= 1);
     MOZ_ASSERT(!cx->compartment()->isAtomsCompartment());
 
     cx->runtime()->gc.gcIfRequested();
 
     // A worker thread may have requested an interrupt after finishing an Ion
     // compilation.
     jit::AttachFinishedCompilations(cx);
 
+    // Don't call the interrupt callback if we only interrupted for GC or Ion.
+    if (!invokeCallback)
+        return true;
+
     // Important: Additional callbacks can occur inside the callback handler
     // if it re-enters the JS engine. The embedding must ensure that the
     // callback is disconnected before attempting such re-entry.
     if (cx->interruptCallbackDisabled)
         return true;
 
     bool stop = false;
     for (JSInterruptCallback cb : cx->interruptCallbacks()) {
@@ -497,43 +501,44 @@ InvokeInterruptCallback(JSContext* cx)
         chars = u"(stack not available)";
     JS_ReportErrorFlagsAndNumberUC(cx, JSREPORT_WARNING, GetErrorMessage, nullptr,
                                    JSMSG_TERMINATED, chars);
 
     return false;
 }
 
 void
-JSContext::requestInterrupt(InterruptMode mode)
+JSContext::requestInterrupt(InterruptReason reason)
 {
-    interrupt_ = true;
+    interruptBits_ |= uint32_t(reason);
     jitStackLimit = UINTPTR_MAX;
 
-    if (mode == JSContext::RequestInterruptUrgent) {
+    if (reason == InterruptReason::CallbackUrgent) {
         // If this interrupt is urgent (slow script dialog for instance), take
         // additional steps to interrupt corner cases where the above fields are
-        // not regularly polled. Wake Atomics.wait() and irregexp JIT code.
-        interruptRegExpJit_ = true;
+        // not regularly polled.
         FutexThread::lock();
         if (fx.isWaiting())
             fx.wake(FutexThread::WakeForJSInterrupt);
         fx.unlock();
         wasm::InterruptRunningCode(this);
     }
 }
 
 bool
 JSContext::handleInterrupt()
 {
     MOZ_ASSERT(CurrentThreadCanAccessRuntime(runtime()));
-    if (interrupt_ || jitStackLimit == UINTPTR_MAX) {
-        interrupt_ = false;
-        interruptRegExpJit_ = false;
+    if (hasAnyPendingInterrupt() || jitStackLimit == UINTPTR_MAX) {
+        bool invokeCallback =
+            hasPendingInterrupt(InterruptReason::CallbackUrgent) ||
+            hasPendingInterrupt(InterruptReason::CallbackCanWait);
+        interruptBits_ = 0;
         resetJitStackLimit();
-        return InvokeInterruptCallback(this);
+        return HandleInterrupt(this, invokeCallback);
     }
     return true;
 }
 
 bool
 JSRuntime::setDefaultLocale(const char* locale)
 {
     if (!locale)