1524006 - Add a medium-high priority queue between high and normal

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Quite often processing something ahead of normal queue could speed up page load and other stuff.
We don't want to reuse high priority queue, since that is really for vsync.

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: mediumhigh.diff

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 9040272 [details] [diff] [review]
mediumhigh.diff

I don't think we're going to solve anything by adding yet another fine-grained prioritization level.

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Eh, https://bugzilla.mozilla.org/show_bug.cgi?id=1519567#c10

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: medium_high_queue.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87338aa25e5f1a47ee347cf51c154034bdef7a81

As far as I see, we can't really live by having just normal priority queue (+ high prio for vsync), since web pages' scripts run using normal priority, and postMessage etc may flood it and so on.

I'm planning to use medium-high for image stuff and probably also for
certain service worker -> main thread communication (since currently workers don't start any network connections if main thread's normal priority queue is flooding).

The patch is simple, just use medium high queue before normal.
mozilla::CreateMediumHighRunnable is a helper, that let's us try out converting certain runnables easier to use this new queue.

The whitespace changes to SchedulerGroup.* are just because I run clang-format on the whole xpcom/threads

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1519567#c16

Friendly review ping :)

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 9043737 [details] [diff] [review]
medium_high_queue.diff

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

This patch is so gross.  I worry that we're heading down a path of "well, we need a HighMediumHigh priority queue...and we need a MediumMediumHigh priority queue..." and so on, and the whole thing is going to become so fragile that we can't untangle it for fear of regressing benchmarks.  If we want to prioritize particular events for a given document, we should be doing the prioritization at some higher level than the main thread.

::: xpcom/threads/nsIThread.idl
@@ +92,5 @@
>  
>    /**
> +   * Similar to above, but checks only possible medium high priority queue.
> +   */
> +  boolean hasPendingMediumHighPriorityEvents();

This function appears to be unused in this patch and in patches in dependent bugs, please remove it from the interface and implementation.

::: xpcom/threads/nsThreadManager.h
@@ +67,5 @@
>    void ResumeInputEventPrioritization();
>  
>    static bool MainThreadHasPendingHighPriorityEvents();
>  
> +  static bool MainThreadHasPendingMediumHighPriorityEvents();

Likewise for this.

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to Nathan Froyd [:froydnj] from comment #6)

This patch is so gross. I worry that we're heading down a path of "well, we
need a HighMediumHigh priority queue...and we need a MediumMediumHigh
priority queue..." and so on, and the whole thing is going to become so
fragile that we can't untangle it for fear of regressing benchmarks. If we
want to prioritize particular events for a given document, we should be
doing the prioritization at some higher level than the main thread.

Not given document, but in general.
With Fission this will be closer to Quantum DOM, since then we'll have per domain normal queue (running most of the JS).

And even without this patch RefreshDriver isn't per document, so vsync (aka high priority queue) isn't per document either. And input event queue isn't per document either, and can't be and so on. This isn't much different.

@@ +92,5 @@

/**

• • Similar to above, but checks only possible medium high priority queue.
• */
• boolean hasPendingMediumHighPriorityEvents();

This function appears to be unused in this patch and in patches in dependent
bugs, please remove it from the interface and implementation.
ok

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: medium_high_queue_2.diff

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/535b72af7ed8
Add a medium-high priority queue between high and normal, r=froydnj

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/535b72af7ed8