1597158 - Get rid of temporary unlocking in IdlePeriodState

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Boris Zbarsky [:bzbarsky]]

Right now IdlePeriodState does various temporary unlocking to do things like send IPC messages, touch the timer thread, etc.

I looked into options for handling this by doing all the idle stuff after we have unlocked anyway to return from ThreadEventQueue::GetEvent, without doing double-gets, but the problem are cases when we find no event and then sleep until one appears. In those cases, we need to either not do any idle state updates before the sleep (so not do the IdlePeriodState::RanOutOfTasks call) or we need to unlock to do that call, and then have to retry getting a runnable anyway before we can sleep, since someone might have queued one on us.

Patches coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1597158 part 1. Change the GetEvent signature on PrioritizedEventQueue to return whether we considered running idle runnables. r=smaug

The caller will need to know this to properly update idle state.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1597158 part 2. Expose the IdlePeriodState of a PrioritizedEventQueue to its consumers. r=smaug

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1597158 part 3. Expose whether there are any idle runnables on PrioritizedEventQueue. r=smaug

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1597158 part 4. Stop unlocking the provided mutex in IdlePeriodState methods. r=smaug

The basic idea, suggested by Olli, is that we can try to get a runnable in
ThreadEventQueue::GetEvent, and if that does not produce anything unlock our
mutex, do whatever idle state updates we need to do, re-lock our mutex. Then
always we need to try getting a runnable again, because a non-idle runnable
might have gotten queued while we had the lock unlocked. So we can't sleep on
our mutex, in the mayWait case, unless we try to get a runnable again first.

My notes on the current (pre this patch) unlocking setup follow.

There are four places where we currently unlock:

1. IdlePeriodState::GetIdleDeadlineInternal. Needed only when !aIsPeek, to
   RequestIdleToken, which can do IPC. The only caller, via
   GetDeadlineForIdleTask, is PrioritizedEventQueue::GetEvent and only when we
   selected the idle or deferred queue. We need this to set the proper deadline
   on the idle event. In the cases when this unlock happens we currently never
   return an idle event, because if we got here that means that we do not have an
   idle token.

2. IdlePeriodState::GetLocalIdleDeadline. Needs to unlock to get the idle
   period hint. The can get called from GetIdleDeadlineInternal in both cases:
   peek and get. The callstack for the get case is covered above. The peek case
   is called from PrioritizedEventQueue::HasReadyEvent which is called from
   ThreadEventQueue::HasPendingEvent.

3. IdlePeriodState::SetPaused, because it sends an IPC message. This is only
   called from EnsureIsPaused, which is called from:

• IdlePeriodState::GetIdleDeadlineInternal. Only in the !aIsPeek case.
• IdlePeriodState::RanOutOfTasks called from:
  • PrioritizedEventQueue::GetEvent if we fell into the idle case and our
    queues are empty.
  • PrioritizedEventQueue::DidRunEvent if we are empty.

4. IdlePeriodState::ClearIdleToken because it sends an IPC message. This is
   called from:
   • IdlePeriodState::RanOutOfTasks; see SetPaused.
   • IdlePeriodState::GetIdleDeadlineInternal like EnsureIsPaused.
   • IdlePeriodState::GetIdleToken if token is in the past. This is only
     called from GetIdleDeadlineInternal, both cases.
   • IdlePeriodState::FlagNotIdle called from PrioritizedEventQueue::GetEvent
     if we find an event in a non-idle queue.

Or rewriting in terms of API entrypoints on IdlePeriodState that might need to
unlock:

• Anything to do with getting deadlines, whether we are peeking or getting.
  Basically, if we need an updated deadline we need to unlock.
• When we have detected we are completely out of tasks (idle or not) to run.
  Right now we do that when either we're asked for an event and don't have one
  or if we run an event and are empty after that (before unlocking!). But the
  unlocking or not happens in nsThreadEventQueue::DidRunEvent, so separately
  from the getting of the event. In particular, we are unlocked before we
  enter DidRunEvent, and unlock again before we return from it, so we can do
  whatever updates we want there.
• When we have detected that we have a non-idle event to run; this calls
  FlagNotIdle.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3e6fb63c18
part 1.  Change the GetEvent signature on PrioritizedEventQueue to return whether we considered running idle runnables.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/8832002ff7e9
part 2.  Expose the IdlePeriodState of a PrioritizedEventQueue to its consumers.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/19be7795094b
part 3.  Expose whether there are any idle runnables on PrioritizedEventQueue.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/74f9a4d6c5d8
part 4.  Stop unlocking the provided mutex in IdlePeriodState methods.  r=smaug

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7b3e6fb63c18
https://hg.mozilla.org/mozilla-central/rev/8832002ff7e9
https://hg.mozilla.org/mozilla-central/rev/19be7795094b
https://hg.mozilla.org/mozilla-central/rev/74f9a4d6c5d8