Last Comment Bug 1306591 - Add urgent priority queue to nsThread and add an IPC annotation to use it
: Add urgent priority queue to nsThread and add an IPC annotation to use it
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 50 Branch
: Unspecified Unspecified
P3 normal with 1 vote (vote)
: mozilla52
Assigned To: Olli Pettay [:smaug]
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on: 1316575 1316686 1169287 1306593 1306708 1318900 1322970 1329079 1331706
Blocks: gdoc_read_basic_table_1_ubuntu(45.25%) QuantumDOM 1315570
  Show dependency treegraph
 
Reported: 2016-09-30 04:59 PDT by Olli Pettay [:smaug]
Modified: 2017-02-16 06:08 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
wip (16.54 KB, patch)
2016-10-07 14:11 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
wip2 (16.54 KB, patch)
2016-10-10 04:03 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v3 (24.72 KB, patch)
2016-10-20 15:30 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v5 (25.41 KB, patch)
2016-10-26 13:43 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v6, fix for nsEventQueue::GetEvent (27.14 KB, patch)
2016-10-28 01:55 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v7 (18.75 KB, patch)
2016-11-05 10:23 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v8 (18.58 KB, patch)
2016-11-05 11:47 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v9 (18.57 KB, patch)
2016-11-05 16:40 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
ipc_priority_11.diff (18.11 KB, patch)
2016-11-06 12:14 PST, Olli Pettay [:smaug]
wmccloskey: review+
Details | Diff | Splinter Review
v12 (merge on top of bkelly's patches) (18.84 KB, patch)
2016-11-07 13:49 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v13 (20.03 KB, patch)
2016-11-07 17:34 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] 2016-09-30 04:59:09 PDT
Thinker, could you move your patch from bug 1269690 perhaps here?
Comment 1 User image Olli Pettay [:smaug] 2016-09-30 11:36:08 PDT
Given the comments in the mailing list, perhaps we could initially add a new annotation and do possible renaming of urgent and high later.

The new one could be maybe...  priority_queue ?

Though, we don't have that many urgent or high usage, so I guess renaming first might be simple enough, and then just have prio(urgent) to have this special mean to use priority queue on the receiving side. prio(high) could be removed.
Comment 2 User image Bill McCloskey (:billm) 2016-09-30 17:04:38 PDT
Thinker, I added a new prio() annotation in bug 1306708 (and renamed the old prio() to nested()). You should be able to access this priority on the IPC message object. I think this will make things a lot simpler for you.
Comment 3 User image Olli Pettay [:smaug] 2016-10-01 06:11:36 PDT
I guess we can accept this new prio(urgent) to work always, both parent->child and child->parent.
It just means which queue to use on the receiving side, right?
Comment 4 User image Bill McCloskey (:billm) 2016-10-01 10:14:14 PDT
Yes, it will always work in both directions. It has whatever meaning we decide to give it. I only added one priority, prio(high). We can add more if we need them.
Comment 5 User image Olli Pettay [:smaug] 2016-10-07 14:11:44 PDT
Created attachment 8799039 [details] [diff] [review]
wip

This is the patch from bug 1269690 updated to use the current ipdl annotations.
VSync is has higher priority.

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4437d26dc98f2b144f78bbbb2479637a956cc63c
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=4437d26dc98f2b144f78bbbb2479637a956cc63c
Comment 6 User image Olli Pettay [:smaug] 2016-10-10 03:21:55 PDT
Need to fix that double mRefCount member issue, but surprisingly good results. Couple of tests to fix, or need to fix the underlying implementation.
Comment 7 User image Olli Pettay [:smaug] 2016-10-10 04:03:48 PDT
Created attachment 8799360 [details] [diff] [review]
wip2

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=152f018659d6188f74110ad64be8c85f6623e0cb
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=152f018659d6188f74110ad64be8c85f6623e0cb
Comment 8 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-10-10 14:04:17 PDT
What do we expect to use this for?
Comment 9 User image Olli Pettay [:smaug] 2016-10-10 14:05:27 PDT
vsync and user input will use high priority queue, possibly also some tab switching stuff.
Comment 13 User image Olli Pettay [:smaug] 2016-10-26 13:43:30 PDT
Created attachment 8804894 [details] [diff] [review]
v5

devtools run super heavy JS in rAF so had to increase timeout value (for debug builds).
 
test_pathAnimInterpolation.xhtml has clearly a bug, since it doesn't remove all those animation tests, so the next tick will still go through all the animations, and with the patch
it is possible to get a new tick before the window has been unloaded.

I'm not too happy with nsIRunnablePriority usage. I'd rather have NS_HighPriorityDispatchToCurrentThread or some such, but the ipc stuff is messy,
and nsIRunnablePriority works too, for now. If the ipc gets cleaned up, we could simplify this.

mNumberOfHighPriorityRunnablesToProcess is a bit weird stuff, but it is there to ensure we process some non-high priority events too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87e47522f2ab1359c33af9307d166a215195e49d

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=87e47522f2ab1359c33af9307d166a215195e49d
Comment 14 User image Olli Pettay [:smaug] 2016-10-27 02:34:46 PDT
Comment on attachment 8804894 [details] [diff] [review]
v5

See comment 13
Comment 15 User image Bill McCloskey (:billm) 2016-10-27 16:15:21 PDT
Comment on attachment 8804894 [details] [diff] [review]
v5

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

I noticed some issues with the thread stuff. I also think it makes a lot more sense to do this on top of bug 1312960. The MessageChannel stuff isn't correct without that bug, and it will be a lot simpler too. You should just be able to add a GetPriority method to MessageTask that looks at the Message and then you'll be done.

::: xpcom/threads/nsIRunnable.idl
@@ +18,5 @@
>      void run();
>  };
> +
> +[uuid(e75aa42a-80a9-11e6-afb5-e89d87348e2c)]
> +interface nsIRunnablePriority : nsISupports

What about using nsISupportsPriority instead?

::: xpcom/threads/nsThread.h
@@ +162,5 @@
> +      // We don't want to wait if mHighPriorityQueue might have some events.
> +      bool reallyMayWait =
> +        aMayWait && !mHighPriorityQueue->HasPendingEvent(aProofOfLock);
> +      bool retVal =
> +        mNormalPriorityQueue->GetEvent(reallyMayWait, aEvent, aProofOfLock);

I don't understand how this works. Let's say that both queues are empty and we end up waiting on the condition variable (i.e., reallyMayWait is true). Now someone puts an event into the high priority queue. The normal queue wakes up (since it uses the same condition variable), but it immediately goes back to sleep again because its queue is empty:
http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/xpcom/threads/nsEventQueue.cpp#55

I think we're going to have to do the condition variable wait ourselves and then check HasPendingEvent on each queue.

I looked at the patch in bug 1198381 and I think it has a related problem: if we go to sleep waiting for a normal event, we won't wake up if an idle event is dispatched.

I think it would be good for this code to fit better with the code from bug 1198381 and to fix these problems.

@@ +165,5 @@
> +      bool retVal =
> +        mNormalPriorityQueue->GetEvent(reallyMayWait, aEvent, aProofOfLock);
> +
> +      // Let's see if we should next time process some high priority events.
> +      mNumberOfHighPriorityRunnablesToProcess =

The way that mNumberOfHighPriorityRunnablesToProcess is updated seems wrong to me. PutEvent never increments it. What happens if we enter GetEvent and there are high priority events but mNumberOfHighPriorityRunnablesToProcess == 0? We'll first call mNormalPriorityQueue->GetEvent. Assume it returns something (say there were also normal priority events available). Then we'll return a normal priority event even though the high-priority queue is non-empty.
Comment 16 User image Olli Pettay [:smaug] 2016-10-27 16:39:37 PDT
(In reply to Bill McCloskey (:billm) from comment #15)
> I noticed some issues with the thread stuff. I also think it makes a lot
> more sense to do this on top of bug 1312960.
yeah, I was wonder what will happen with that stuff. didn't know if there was a bug already.

> The MessageChannel stuff isn't
> correct without that bug, and it will be a lot simpler too.
how so?

> What about using nsISupportsPriority instead?
Makes no sense since we support only normal and high priority.
I'd like to get rid of nsIRunnablePriority assuming IPC level gets sanitized so that dispatching explicitly 
high priority runnables doesn't make the code too ugly.


> I don't understand how this works. Let's say that both queues are empty and
> we end up waiting on the condition variable (i.e., reallyMayWait is true).
> Now someone puts an event into the high priority queue. The normal queue
> wakes up (since it uses the same condition variable), but it immediately
> goes back to sleep again because its queue is empty:
> http://searchfox.org/mozilla-central/rev/
> 4012e61cc38758ffa1f7ce26039173f67b048853/xpcom/threads/nsEventQueue.cpp#55
Oh, indeed, that while loop is buggy.


> I think we're going to have to do the condition variable wait ourselves and
> then check HasPendingEvent on each queue.
Why? I think it is better to just modify GetEvent, since it is just buggy right now.
IsEmpty() doesn't make sense there.


> I looked at the patch in bug 1198381 and I think it has a related problem:
> if we go to sleep waiting for a normal event, we won't wake up if an idle
> event is dispatched.
Ah, that is possible.
 
> I think it would be good for this code to fit better with the code from bug
> 1198381 and to fix these problems.
I started to write a patch following the patterns from that bug, but our IPC code is too messy that it wasn't really possible without making code even more messy


> The way that mNumberOfHighPriorityRunnablesToProcess is updated seems wrong
> to me. PutEvent never increments it.
That is the whole point that PutEvent doesn't update it. We update it only after processing one normal event.


> What happens if we enter GetEvent and
> there are high priority events but mNumberOfHighPriorityRunnablesToProcess
> == 0?
We should process normal priority event.


> We'll first call mNormalPriorityQueue->GetEvent. Assume it returns
> something (say there were also normal priority events available). Then we'll
> return a normal priority event even though the high-priority queue is
> non-empty.
Yes, that is the whole point. We must process also non-high priority events even though there are high priority events pending.
The way to decide how often is of course a question. Initially I had a boolean to ensure after each high priority event one normal event is processed, but that felt even too conservative.
Comment 17 User image Bill McCloskey (:billm) 2016-10-27 16:50:36 PDT
(In reply to Olli Pettay [:smaug] from comment #16)
> > The MessageChannel stuff isn't
> > correct without that bug, and it will be a lot simpler too.
> how so?

Right now the IPC code doesn't have a direct link between runnables and messages. Most of the time the runnable we dispatch for a message ends up handling that message. But sometimes it handles a different message.

> > What about using nsISupportsPriority instead?
> Makes no sense since we support only normal and high priority.
> I'd like to get rid of nsIRunnablePriority assuming IPC level gets sanitized
> so that dispatching explicitly 
> high priority runnables doesn't make the code too ugly.

OK. Were you thinking of changing the Dispatch call or something?

> > I think we're going to have to do the condition variable wait ourselves and
> > then check HasPendingEvent on each queue.
> Why? I think it is better to just modify GetEvent, since it is just buggy
> right now.
> IsEmpty() doesn't make sense there.

I think we need to call IsEmpty(). Wait() can wake up even if the condition variable hasn't been notified.

> > I think it would be good for this code to fit better with the code from bug
> > 1198381 and to fix these problems.
> I started to write a patch following the patterns from that bug, but our IPC
> code is too messy that it wasn't really possible without making code even
> more messy

I meant the requestIdleCallback bug. I think we should use the same mechanism for high priority tasks as we use for idle tasks.

> > The way that mNumberOfHighPriorityRunnablesToProcess is updated seems wrong
> > to me. PutEvent never increments it.
> That is the whole point that PutEvent doesn't update it. We update it only
> after processing one normal event.

Oh, sorry, I missed that part of comment 13. Can you add some comments about how that is supposed to work?
Comment 18 User image Olli Pettay [:smaug] 2016-10-28 00:55:24 PDT
(In reply to Bill McCloskey (:billm) from comment #17)

> > > What about using nsISupportsPriority instead?
> > Makes no sense since we support only normal and high priority.
> > I'd like to get rid of nsIRunnablePriority assuming IPC level gets sanitized
> > so that dispatching explicitly 
> > high priority runnables doesn't make the code too ugly.
> 
> OK. Were you thinking of changing the Dispatch call or something?
Yeah, explicit NS_HighPriorityDispatchToCurrentThread or some such


> 
> > > I think we're going to have to do the condition variable wait ourselves and
> > > then check HasPendingEvent on each queue.
> > Why? I think it is better to just modify GetEvent, since it is just buggy
> > right now.
> > IsEmpty() doesn't make sense there.
> 
> I think we need to call IsEmpty(). Wait() can wake up even if the condition
> variable hasn't been notified.

Sorry, my comment was vague. I mean we don't need to have IsEmpty() in 'while'. 'if' is enough


> > > I think it would be good for this code to fit better with the code from bug
> > > 1198381 and to fix these problems.
> > I started to write a patch following the patterns from that bug, but our IPC
> > code is too messy that it wasn't really possible without making code even
> > more messy
> 
> I meant the requestIdleCallback bug. I think we should use the same
> mechanism for high priority tasks as we use for idle tasks.
I actually think we'll need to move rIC closer to the approach taken here.
Though, rIC is quite different. high priority and normal priority events are interleaved in a bit different way.



> Oh, sorry, I missed that part of comment 13. Can you add some comments about
> how that is supposed to work?
Definitely.
Comment 19 User image Olli Pettay [:smaug] 2016-10-28 01:09:06 PDT
Hmm, perhaps because of nsThreadPool stuff condvar handling might be simpler to do outside nsEventQueue after all. Or have two modes in nsEventQueue, normal mode and threadpool mode which lets one to use
those hacky Wait() and NotifyAll() methods defined in the .h
Comment 20 User image Olli Pettay [:smaug] 2016-10-28 01:55:07 PDT
Created attachment 8805491 [details] [diff] [review]
v6, fix for nsEventQueue::GetEvent

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f96a11179467be593959abf8e7c7305b7bed9b

But sounds like ipc part should be done after bug 1312960. And if that let's us to get rid of nsIRunnablePriority, even better.
Comment 21 User image Olli Pettay [:smaug] 2016-11-05 10:23:54 PDT
Created attachment 8807889 [details] [diff] [review]
v7

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=02ecb3191d3d08c69195868b99d58e49b62ff1dc
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=02ecb3191d3d08c69195868b99d58e49b62ff1dc
Comment 22 User image Olli Pettay [:smaug] 2016-11-05 11:47:52 PDT
Created attachment 8807894 [details] [diff] [review]
v8

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=390120bd06099c70be673b043d414e78ef644d6b
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=390120bd06099c70be673b043d414e78ef644d6b
Comment 23 User image Olli Pettay [:smaug] 2016-11-05 16:40:23 PDT
Created attachment 8807921 [details] [diff] [review]
v9

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0512c4a522bfd7b56482ea86dbeb0aa44022c76
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a0512c4a522bfd7b56482ea86dbeb0aa44022c76
Comment 24 User image Olli Pettay [:smaug] 2016-11-06 12:14:19 PST
Created attachment 8807985 [details] [diff] [review]
ipc_priority_11.diff

Explicitly not trying to fix bug 1313988 here.
Comment 26 User image Olli Pettay [:smaug] 2016-11-07 13:49:05 PST
Created attachment 8808364 [details] [diff] [review]
v12 (merge on top of bkelly's patches)

Since this + bkelly's changes to setTimeout handling could easily break more tests, better to push to try again.
No real code changes from v11, except renaming the values in nsEventQueue's enum, since the new ThrottledEventQueue uses them too.

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb01d26e3b0c1a663c60dc71b98b1406623a3b9e
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=fb01d26e3b0c1a663c60dc71b98b1406623a3b9e
Comment 27 User image Bill McCloskey (:billm) 2016-11-07 14:03:38 PST
Comment on attachment 8807985 [details] [diff] [review]
ipc_priority_11.diff

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

::: ipc/glue/MessageChannel.h
@@ +467,5 @@
>          NS_IMETHOD Run() override;
>          nsresult Cancel() override;
> +        NS_IMETHOD GetPriority(uint32_t* aPriority) override
> +        {
> +          *aPriority = mMessage.priority();

I'd feel a lot better if you did something like this:
  *aPriority = mMessage.priority() == Message::HIGH_PRIORITY
             ? PRIORITY_HIGH
             : PRIORITY_NORMAL;
It's possible we'll add a new priority to one side or the other and then these might not match up.

Also, can you move this function to the .cpp file? That way someone won't miss it while reading through the .cpp file.

::: xpcom/threads/nsEventQueue.cpp
@@ +25,5 @@
>    , mOffsetHead(0)
>    , mOffsetTail(0)
> +  , mType(aType)
> +{
> +  mOwnEventsAvailable =

I feel like this would be cleaner if nsThread/nsThreadPool always owned the CondVar and just passed it in here. Then you would only need one constructor and the ownership issues would be simpler.

::: xpcom/threads/nsThread.h
@@ +151,3 @@
>      }
>  
>      bool GetEvent(bool aMayWait, nsIRunnable** aEvent,

Can you move GetEvent/PutEvent to the .cpp file? They're so big that inlining won't help, and it'll be easier to maintain the code in the future.

@@ +176,5 @@
> +        if (*aEvent) {
> +          // We got an event, return early.
> +          return retVal;
> +        }
> +

Please remove the blank line.

@@ +177,5 @@
> +          // We got an event, return early.
> +          return retVal;
> +        }
> +
> +      } while(aMayWait || mProcessHighPriorityRunnable);

Space after "while".

@@ +184,5 @@
>      }
>  
>      void PutEvent(nsIRunnable* aEvent, mozilla::MutexAutoLock& aProofOfLock)
>      {
> +      nsCOMPtr<nsIRunnablePriority> runnablePrio = do_QueryInterface(aEvent);

It seems like this code duplication is to avoid an extra refcount. However, it looks like nsEventQueue::PutEvent(nsIRunnable*) takes an extra ref anyway. Why not do it sooner here and just call into the already_AddRefed version?

@@ +233,5 @@
> +
> +    // Try to process one high priority runnable after each normal
> +    // priority runnable. This gives the processing model HTML spec has for
> +    // 'Update the rendering' in the case only vsync messages are in the high
> +    // priority queue and prevents starving the normal priority queue.

Given how this works, it seems a bit weird to me to call these "high priority" events since we actually process the normal priority events in preference to high priority events. But I can't think of a better name, so I guess this is okay for now.
Comment 28 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-11-07 14:07:47 PST
(In reply to Bill McCloskey (:billm) from comment #27)
> @@ +233,5 @@
> > +
> > +    // Try to process one high priority runnable after each normal
> > +    // priority runnable. This gives the processing model HTML spec has for
> > +    // 'Update the rendering' in the case only vsync messages are in the high
> > +    // priority queue and prevents starving the normal priority queue.
> 
> Given how this works, it seems a bit weird to me to call these "high
> priority" events since we actually process the normal priority events in
> preference to high priority events. But I can't think of a better name, so I
> guess this is okay for now.

Instead of baking this into nsThread could we instead make nsRefreshDriver call NS_ProcessNextEvent() to force some normal runnables if it detects that requestAnimationFrame took a long time?  That would keep the weird logic close to the thing causing the weird logic.
Comment 29 User image Olli Pettay [:smaug] 2016-11-07 14:10:14 PST
(In reply to Ben Kelly [:bkelly] from comment #28)
> Instead of baking this into nsThread could we instead make nsRefreshDriver
> call NS_ProcessNextEvent() to force some normal runnables if it detects that
> requestAnimationFrame took a long time?  That would keep the weird logic
> close to the thing causing the weird logic.
I really prefer to follow the spec model as much as possible here.
Comment 30 User image Olli Pettay [:smaug] 2016-11-07 14:14:05 PST
(In reply to Bill McCloskey (:billm) from comment #27)
> > +  mOwnEventsAvailable =
> 
> I feel like this would be cleaner if nsThread/nsThreadPool always owned the
> CondVar and just passed it in here. Then you would only need one constructor
> and the ownership issues would be simpler.
possibly yes.
ThrottledEventQueue added a new use for this, but I guess I could just add the condvar as its member variable.


> 
> Can you move GetEvent/PutEvent to the .cpp file? They're so big that
> inlining won't help, and it'll be easier to maintain the code in the future.
> 
GetEvent at least. PutEvents are small

> > +
> > +    // Try to process one high priority runnable after each normal
> > +    // priority runnable. This gives the processing model HTML spec has for
> > +    // 'Update the rendering' in the case only vsync messages are in the high
> > +    // priority queue and prevents starving the normal priority queue.
> 
> Given how this works, it seems a bit weird to me to call these "high
> priority" events since we actually process the normal priority events in
> preference to high priority events. But I can't think of a better name, so I
> guess this is okay for now.
right. The high priority comes from the fact that this type of messages should be used very rarely.
Comment 31 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-11-07 14:17:11 PST
(In reply to Olli Pettay [:smaug] from comment #30)
> (In reply to Bill McCloskey (:billm) from comment #27)
> > > +  mOwnEventsAvailable =
> > 
> > I feel like this would be cleaner if nsThread/nsThreadPool always owned the
> > CondVar and just passed it in here. Then you would only need one constructor
> > and the ownership issues would be simpler.
> possibly yes.
> ThrottledEventQueue added a new use for this, but I guess I could just add
> the condvar as its member variable.

By this you mean not having a nested runnable?  Ok.

We could still hint to nsThread from nsRefreshDriver that high priority should be suspended for N runnables or time period, though.  It just feels like a layering violation to have this special logic in the deep guts of the system for a particular condition occurring in a higher layer.
Comment 32 User image Olli Pettay [:smaug] 2016-11-07 14:25:57 PST
what is the violation? We have two queues, of which one is commonly used and once is rarely used, yet we give them both effectively the same priority, meaning that runnables in the rarely used get processed faster.
Comment 33 User image Bill McCloskey (:billm) 2016-11-07 14:32:17 PST
What if we call it a secondary queue instead of a high-priority queue?
Comment 34 User image Ben Kelly [not reviewing due to deadline][:bkelly] 2016-11-07 14:36:03 PST
(In reply to Olli Pettay [:smaug] from comment #32)
> what is the violation? We have two queues, of which one is commonly used and
> once is rarely used, yet we give them both effectively the same priority,
> meaning that runnables in the rarely used get processed faster.

Because of requestAnimationFrame() in queue 1 depending on logic running in js in queue 2 you cannot really implement this as a high level priority queue.  If you made the refresh driver in queue 1 hint that queue 2 needs to run, though, then nsThread would not need to guess when it should force the high priority mechanism off.  It would be more explicit and more precise.

Anyway, it was just a suggestion.
Comment 35 User image Olli Pettay [:smaug] 2016-11-07 17:34:31 PST
Created attachment 8808436 [details] [diff] [review]
v13

I like "secondary queue". 

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5487c6761e55f0f9ab8d267126afee6090f93615
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=5487c6761e55f0f9ab8d267126afee6090f93615
Comment 36 User image Pulsebot 2016-11-08 04:41:43 PST
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab432069073
add secondary event queue to let high priority messages to be processed sooner, r=billm
Comment 37 User image Wes Kocher (:KWierso) 2016-11-08 14:00:16 PST
https://hg.mozilla.org/mozilla-central/rev/fab432069073

Note You need to log in before you can comment on or make changes to this bug.