1742072 - Make nsThreadPool::ShutdownWithTimeout use the standard SpinEventLoopUntil and a timer

Status: NEW

(Core :: XPCOM, task)

Description

[Jens Stutte [:jstutte]]

SpinMTEventLoopUntil is a slight modification of SpinEventLoopUntil with a timeout.

It should be replaced by the use of a normal SpinEventLoopUntil and a timer, see QuotaManager::Shutdown for an example. Otherwise we do not benefit from annotations and/or profile markers added recently.

Comment 1

[Valentin Gosu [:valentin]]

The reason we didn't use SpinEventLoop before was:
(In reply to Nathan Froyd [:froydnj] from bug 1500861 comment #7)

SpinEventLoopUntil does indeed block, so if there are no events, that'd be a
problem. That's exactly what's getting us into trouble with the shutdown
hangs: we're expecting an nsThreadShutdownAckEvent to be dispatched to our
event loop while we're spinning, but the thread getting shutdown is hanging
somewhere and not able to dispatch the event, so we never see that event.

So we'd need to do something more akin to what NS_ProcessPendingEvents does
(i.e. spin by not blocking when calling ProcessNextEvent).

So what SpinMTEventLoopUntil does is it processes MT event loop events one by one.
If there are none, it sleeps for one second (to avoid 100% CPU 🙂) until the timeout finally expires.

The replacement code should have similar properties.

Comment 2

[Valentin Gosu [:valentin]]

One a second thought using a timer would probably fix the "there are no events" issue since there'd eventually be an event when the timer fires.
We still need to sleep when there are no events, so the resulting code would be something like:

do {
  if (!SpinEventLoopUntil("reason"_ns,[&]() {return mTimerFired;}) {
     // Sleep if there are no events to process to avoid 100% CPU usage.
     PR_Sleep(PR_MillisecondsToInterval(1));
  }
} while (!mTimerFired);

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #1)

The reason we didn't use SpinEventLoop before was:
(In reply to Nathan Froyd [:froydnj] from bug 1500861 comment #7)

SpinEventLoopUntil does indeed block, so if there are no events, that'd be a
problem. That's exactly what's getting us into trouble with the shutdown
hangs: we're expecting an nsThreadShutdownAckEvent to be dispatched to our
event loop while we're spinning, but the thread getting shutdown is hanging
somewhere and not able to dispatch the event, so we never see that event.

I am not sure what we can do differently about this if we do not use SpinEventLoopUntil. In the end we need to test some shared state that tells us if we are done. That state is usually flipped by an event (but could be anything), so we would block in any kind of loop, IIUC?

So we'd need to do something more akin to what NS_ProcessPendingEvents does
(i.e. spin by not blocking when calling ProcessNextEvent).

So what SpinMTEventLoopUntil does is it processes MT event loop events one by one.
If there are none, it sleeps for one second (to avoid 100% CPU 🙂) until the timeout finally expires.

The replacement code should have similar properties.

I read PR_Sleep(PR_MillisecondsToInterval(1)); as 1 millisecond timeout., not one second, hopefully. Still this throttles the main thread event queue to some extent. Is this intended an would be part of the "similar properties" ?

(In reply to Valentin Gosu [:valentin] (he/him) from comment #2)

One a second thought using a timer would probably fix the "there are no events" issue since there'd eventually be an event when the timer fires.
We still need to sleep when there are no events, so the resulting code would be something like:

do {
  if (!SpinEventLoopUntil("reason"_ns,[&]() {return mTimerFired;}) {
     // Sleep if there are no events to process to avoid 100% CPU usage.
     PR_Sleep(PR_MillisecondsToInterval(1));
  }
} while (!mTimerFired);

I do not think we need the outer loop here if it has just the same condition as the predicate?

Comment 4

[Valentin Gosu [:valentin]]

(In reply to Jens Stutte [:jstutte] from comment #3)

I read PR_Sleep(PR_MillisecondsToInterval(1)); as 1 millisecond timeout., not one second, hopefully. Still this throttles the main thread event queue to some extent. Is this intended an would be part of the "similar properties" ?

I meant 1ms 🙂. Indeed, as long as the timer fires eventually, we don't need to sleep anymore.

I do not think we need the outer loop here if it has just the same condition as the predicate?

Right. I had a different mental model of how SpinEventLoopUntil worked. We don't need a loop.