Fix some LabeledEventQueue bugs

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

I found some bugs in LabeledEventQueue during testing today.
Created attachment 8906222 [details] [diff] [review]
Unlabeled epochs should never cause labeled events to be returned

If an epoch is unlabeled, we should only return unlabeled events. Currently, though, we fall through to labeled events here, if IsReadyToRun is false:
http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/xpcom/threads/LabeledEventQueue.cpp#134

This isn't actually a problem now since IsReadyToRun always returns true when the scheduler is disabled. But it needs to be fixed.
Attachment #8906222 - Flags: review?(nfroyd)
Attachment #8906222 - Flags: review?(nfroyd) → review+
Created attachment 8906447 [details] [diff] [review]
Make linked list of SchedulerGroups static

This was kind of an embarrassing bug. I was using a linked list threaded through SchedulerGroups in each LabeledEventQueue. If there are multiple LabeledEventQueues, they'll share the same next/prev pointers in SchedulerGroup and everything will get messed up. I didn't notice this until Stone turned on input event prioritization. Until then, none of the events outside of the normal priority queue were labeled, so this didn't really come up.

To fix this, I made the linked list of SchedulerGroups be static (so there's only one per process). The LabeledEventQueues have to cooperate a little bit when using this queue. Since they're all on the main thread, this isn't really an issue.

I also took the opportunity to remove some of the list insertion/removal operations we have to do by tracking the current position in the list to remove events from. This way we just move this pointer circularly over the list as we process events, instead of popping things from the front and moving them to the end each time.
Attachment #8906447 - Flags: review?(nfroyd)
Created attachment 8906448 [details] [diff] [review]
enable multiple event queues

With these patches, everything looks green to turn on multiple event queues.
Attachment #8906448 - Flags: review?(nfroyd)
Comment on attachment 8906448 [details] [diff] [review]
enable multiple event queues

Review of attachment 8906448 [details] [diff] [review]:
-----------------------------------------------------------------

Buckle up!
Attachment #8906448 - Flags: review?(nfroyd) → review+
Comment on attachment 8906447 [details] [diff] [review]
Make linked list of SchedulerGroups static

Review of attachment 8906447 [details] [diff] [review]:
-----------------------------------------------------------------

This is a lot less nice than the mSchedulerGroup code we had before. :(

I think everything makes sense, but I have questions, below.

::: xpcom/threads/LabeledEventQueue.cpp
@@ +115,5 @@
>    queue->Push(QueueEntry(event.forget(), epoch->mEpochNumber));
>  
> +  if (group) {
> +    group->EnqueueEvent();
> +    if (group->CountEvents() == 1) {

WDYT about having EnqueueEvent() return a status (an enum would be nice, but a bool is mostly acceptable) to indicate whether we enqueued the first event?  That way we don't have to expose this CountEvents() method, which sounds expensive, even though it's actually not.

@@ +143,5 @@
>  }
>  
> +// Returns the next SchedulerGroup after |aGroup| in sSchedulerGroups. Wraps
> +// around to the beginning of the list when we hit the end.
> +SchedulerGroup*

Nit: make this static?

@@ +205,5 @@
> +  // Iterate over each SchedulerGroup once, starting at sCurrentSchedulerGroup.
> +  size_t count = 0;
> +  SchedulerGroup* firstGroup = sCurrentSchedulerGroup;
> +  for (SchedulerGroup* group = firstGroup; !count || group != firstGroup;
> +       group = NextSchedulerGroup(group)) {

I almost think keeping the do-while structure that was there previously and using a `goto` for any `continue`'ing that you would do is preferable to this weird `count` variable.

@@ +211,4 @@
>      mAvoidActiveTabCount--;
> +
> +    RunnableEpochQueue* queue = mLabeled.Get(group);
> +    if (!queue) {

This looks like a secondary fix...why did we MOZ_ASSERT we had a queue before, but now it's OK if we don't?  Oh, this is checking to make sure that the group actually has events associated with this queue?  Maybe a comment to that effect would make things clearer?

@@ +224,4 @@
>        PopEpoch();
>  
> +      group->DequeueEvent();
> +      if (group->CountEvents() == 0) {

Same question here about DequeueEvents, but returning the status of whether we're completely drained the queue.

@@ +226,5 @@
> +      group->DequeueEvent();
> +      if (group->CountEvents() == 0) {
> +        // Now we can take group out of sSchedulerGroups.
> +        if (sCurrentSchedulerGroup == group) {
> +          MOZ_ASSERT(group->getNext() == nullptr);

This code doesn't make a lot of sense to me:  We already set sCurrentSchedulerGroup to NextSchedulerGroup(), above, so sCurrentSchedulerGroup == group should...never happen, right?  Oh, perhaps if the group was the only thing in the list, that's what we're catching here?  And therefore if it was the only thing in the list, then obviously there's no next...maybe we should assert getPrev() == nullptr, too, to try and make it a little more clear?

::: xpcom/threads/LabeledEventQueue.h
@@ +29,5 @@
>  class LabeledEventQueue final : public AbstractEventQueue
>  {
>  public:
>    LabeledEventQueue();
> +  virtual ~LabeledEventQueue();

Why are we making this virtual?  It doesn't have to be marked as such, no?
Attachment #8906447 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Comment on attachment 8906447 [details] [diff] [review]
> Make linked list of SchedulerGroups static
> 
> Review of attachment 8906447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a lot less nice than the mSchedulerGroup code we had before. :(

Yes, I agree. I'm trying to think of a good way to re-write this code, but so far I haven't thought of too much.

> ::: xpcom/threads/LabeledEventQueue.cpp
> @@ +115,5 @@
> >    queue->Push(QueueEntry(event.forget(), epoch->mEpochNumber));
> >  
> > +  if (group) {
> > +    group->EnqueueEvent();
> > +    if (group->CountEvents() == 1) {
> 
> WDYT about having EnqueueEvent() return a status (an enum would be nice, but
> a bool is mostly acceptable) to indicate whether we enqueued the first
> event?  That way we don't have to expose this CountEvents() method, which
> sounds expensive, even though it's actually not.

That sounds fine. I originally wrote the code this way (with just a bool), but I couldn't think of a great name to express what EnqueueEvent does. But an enum would make it clearer.

> @@ +211,4 @@
> >      mAvoidActiveTabCount--;
> > +
> > +    RunnableEpochQueue* queue = mLabeled.Get(group);
> > +    if (!queue) {
> 
> This looks like a secondary fix...why did we MOZ_ASSERT we had a queue
> before, but now it's OK if we don't?  Oh, this is checking to make sure that
> the group actually has events associated with this queue?  Maybe a comment
> to that effect would make things clearer?

Yes, now that there's one list for everyone, it's possible to be in the list but not actually have anything in mLabeled.

> @@ +226,5 @@
> > +      group->DequeueEvent();
> > +      if (group->CountEvents() == 0) {
> > +        // Now we can take group out of sSchedulerGroups.
> > +        if (sCurrentSchedulerGroup == group) {
> > +          MOZ_ASSERT(group->getNext() == nullptr);
> 
> This code doesn't make a lot of sense to me:  We already set
> sCurrentSchedulerGroup to NextSchedulerGroup(), above, so
> sCurrentSchedulerGroup == group should...never happen, right?  Oh, perhaps
> if the group was the only thing in the list, that's what we're catching
> here?  And therefore if it was the only thing in the list, then obviously
> there's no next...maybe we should assert getPrev() == nullptr, too, to try
> and make it a little more clear?

Yes, this is correct. I can assert about getPrev as well (and make a comment).

> ::: xpcom/threads/LabeledEventQueue.h
> @@ +29,5 @@
> >  class LabeledEventQueue final : public AbstractEventQueue
> >  {
> >  public:
> >    LabeledEventQueue();
> > +  virtual ~LabeledEventQueue();
> 
> Why are we making this virtual?  It doesn't have to be marked as such, no?

I guess not since ~AbstractEventQueue is virtual.

Comment 7

a year ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96767196afa8
Make linked list of SchedulerGroups static (r=froydnj)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8105474157
Fix LabeledEventQueue bug with unlabeled events (r=froydnj)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9397d2b650e8
Enable picking events from multiple labeled event queues (r=froydnj)
Blocks: 1396155
Blocks: 1398227
You need to log in before you can comment on or make changes to this bug.