Closed Bug 1597158 Opened 11 months ago Closed 10 months ago

Get rid of temporary unlocking in IdlePeriodState

Categories

(Core :: XPCOM, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files)

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.

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.
  1. 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.
Pushed by bzbarsky@mozilla.com:
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
Regressions: 1600331
Regressions: 1602469
You need to log in before you can comment on or make changes to this bug.