This bug will have patches for improving LabeledEventQueue, for allowing it to be used without Scheduler, and for enabling it. This will mean we'll have one event queue per SchedulerGroup, and we'll be able to select events from whichever queue makes the most sense.

This patch makes two changes to LabeledEventQueue: - The SchedulerGroups whose queues are non-empty are put in a linked list. When we go to pop an event, we go through this linked list in a round-robin fashion. This should make scheduling a little more fair. We'll process an event from each tab before we go back to the first tab. - ...except that we'll try to move SchedulerGroups associated with active tabs to the front of this queue. I implemented a heuristic that tries to ensure that we process one event from a background tab for each event from a foreground tab. This is roughly the same heuristic we use for vsync runnables compared to normal priority runnables.

Right now, LabeledEventQueue is only used if the scheduler is enabled. This patch allows it to be used even if there's only one main thread. I was able to factor the event loop creation code into a single templated function, which is cleaner than what we had before. Unfortunately, I couldn't think of where to put this function, so I just stuck it in an awkwardly named header. Not sure if that can be improved.

Currently, MessageChannel asserts that messages are processed in the order that they arrived (assuming they don't have a special priority). The patches in this bug totally break that invariant. To make this assertion work requires encoding into it a lot of logic about how the event loop works, and I'd rather not do that. So I think it's best to remove it.

Comment on attachment 8903845 [details] [diff] [review] Allow LabeledEventQueue to be used outside the Scheduler Review of attachment 8903845 [details] [diff] [review]: ----------------------------------------------------------------- This is an improvement, I think. ::: xpcom/threads/Scheduler.cpp @@ +697,2 @@ > RefPtr<nsThread> mainThread; > + if (Scheduler::UseMultipleQueues()) { It's too bad we're not able to factor out of the logic of whether multiple queues should be used, too. I think we could, but it would involve some gnarly templates, which probably makes it not worth it.

Comment on attachment 8903839 [details] [diff] [review] Support active tab prioritization and round-robin scheduling in LabeledEventQueue Review of attachment 8903839 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable, assuming my understanding below is correct. ::: xpcom/threads/LabeledEventQueue.cpp @@ +148,5 @@ > + // a background group) before we prioritize active tabs again. > + mAvoidActiveTabCount += 2; > + > + group->removeFrom(mSchedulerGroups); > + mSchedulerGroups.insertFront(group); From looking at the TabChild code, we don't have to worry about degenerate cases where we continually pop the group from the list and reinsert it, right? At most we'll pass through this loop over GetActiveTabs() once, since we should really only have one element in the returned array. Is that correct?

Comment on attachment 8903847 [details] [diff] [review] Remove MessageChannel ordering assertion Review of attachment 8903847 [details] [diff] [review]: ----------------------------------------------------------------- Blithely removing assertions like this make me nervous. Is it possible that we could at least keep it if the scheduler isn't active? I don't know what the right way to keep it around in a scheduled world is. I can imagine that it's not particularly easy...

Also, this patch series was a lot less painful to review than I expected, which I think means you did a bunch of good work setting up the design!

(In reply to Nathan Froyd [:froydnj] from comment #5) > This seems reasonable, assuming my understanding below is correct. > > ::: xpcom/threads/LabeledEventQueue.cpp > @@ +148,5 @@ > > + // a background group) before we prioritize active tabs again. > > + mAvoidActiveTabCount += 2; > > + > > + group->removeFrom(mSchedulerGroups); > > + mSchedulerGroups.insertFront(group); > > From looking at the TabChild code, we don't have to worry about degenerate > cases where we continually pop the group from the list and reinsert it, > right? At most we'll pass through this loop over GetActiveTabs() once, > since we should really only have one element in the returned array. Is that > correct? We can have multiple tabs returned in GetActiveTabs() (if there are multiple windows open, for example). I don't see how that would lead to problems, though. Can you elaborate?

(In reply to Bill McCloskey (:billm) from comment #8) > (In reply to Nathan Froyd [:froydnj] from comment #5) > > This seems reasonable, assuming my understanding below is correct. > > > > ::: xpcom/threads/LabeledEventQueue.cpp > > @@ +148,5 @@ > > > + // a background group) before we prioritize active tabs again. > > > + mAvoidActiveTabCount += 2; > > > + > > > + group->removeFrom(mSchedulerGroups); > > > + mSchedulerGroups.insertFront(group); > > > > From looking at the TabChild code, we don't have to worry about degenerate > > cases where we continually pop the group from the list and reinsert it, > > right? At most we'll pass through this loop over GetActiveTabs() once, > > since we should really only have one element in the returned array. Is that > > correct? > > We can have multiple tabs returned in GetActiveTabs() (if there are multiple > windows open, for example). I don't see how that would lead to problems, > though. Can you elaborate? Hm, documentation for GetActiveTabs says: // Returns whichever TabChild is currently in the foreground. If there are // multiple TabChilds in the foreground (due to multiple windows being open), // this returns null. This should only be called if HasActiveTabs() returns // true. If things are actually as you describe, can you file a bug on fixing that documentation? I don't know if it would lead to problems, per se, but let's say we have a bunch of active tabs and they all share a TabGroup (maybe this is not actually possible?). We look at the first child and move the TabGroup to the front of the list. Then we look at the second child, pop the tab group off the front of the list, and put it back on. And we do the same thing for all the remaining children. Or we have two major TabGroups, and we spend time switching their places in the list. It just seems like wasted work. Maybe checking if something was at the front of the list before trying to move it would be just as much work, though. Looking at the code again, though, how would a group ever *not* be in a list (the check before we attempt to shuffle things to the front)?

(In reply to Nathan Froyd [:froydnj] from comment #9) > (In reply to Bill McCloskey (:billm) from comment #8) > > (In reply to Nathan Froyd [:froydnj] from comment #5) > > > This seems reasonable, assuming my understanding below is correct. > > > > > > ::: xpcom/threads/LabeledEventQueue.cpp > > > @@ +148,5 @@ > > > > + // a background group) before we prioritize active tabs again. > > > > + mAvoidActiveTabCount += 2; > > > > + > > > > + group->removeFrom(mSchedulerGroups); > > > > + mSchedulerGroups.insertFront(group); > > > > > > From looking at the TabChild code, we don't have to worry about degenerate > > > cases where we continually pop the group from the list and reinsert it, > > > right? At most we'll pass through this loop over GetActiveTabs() once, > > > since we should really only have one element in the returned array. Is that > > > correct? > > > > We can have multiple tabs returned in GetActiveTabs() (if there are multiple > > windows open, for example). I don't see how that would lead to problems, > > though. Can you elaborate? > > Hm, documentation for GetActiveTabs says: > If things are actually as you describe, can you file a bug on fixing that > documentation? Yeah, filed bug 1397403. Sorry about that. > I don't know if it would lead to problems, per se, but let's say we have a > bunch of active tabs and they all share a TabGroup (maybe this is not > actually possible?). We look at the first child and move the TabGroup to > the front of the list. Then we look at the second child, pop the tab group > off the front of the list, and put it back on. And we do the same thing for > all the remaining children. Or we have two major TabGroups, and we spend > time switching their places in the list. It just seems like wasted work. > Maybe checking if something was at the front of the list before trying to > move it would be just as much work, though. All right, I'll check if the element is already in front. That should handle some of these cases (although not all of them). I could keep track of active TabGroups separately. I'm not sure it's worth optimizing that much though. > Looking at the code again, though, how would a group ever *not* be in a list > (the check before we attempt to shuffle things to the front)? The list only contains SchedulerGroups that have events in the queue. So if a tab doesn't have any events in the queue, then it won't be in the linked list.

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8eaa67e815a5 Change MessageChannel ordering assertion (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/0b295c9c9ffd Support active tab prioritization and round-robin scheduling in LabeledEventQueue (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/9967ddf61c89 Allow LabeledEventQueue to be used outside the Scheduler (r=froydnj)

https://hg.mozilla.org/mozilla-central/rev/8eaa67e815a5 https://hg.mozilla.org/mozilla-central/rev/0b295c9c9ffd https://hg.mozilla.org/mozilla-central/rev/9967ddf61c89