Closed Bug 1524006 Opened Last year Closed 11 months ago

Add a medium-high priority queue between high and normal

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

Attached patch mediumhigh.diff (obsolete) — Splinter Review
Assignee: nobody → bugs
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.
Attachment #9040272 - Flags: feedback-

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

Attachment #9040272 - Attachment is obsolete: true
Attachment #9043737 - Flags: review?(nfroyd)
Blocks: 1519567
Blocks: 1522316
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.
Attachment #9043737 - Flags: review?(nfroyd) → review+

(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

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/535b72af7ed8
Add a medium-high priority queue between high and normal, r=froydnj
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
See Also: → 1552643
You need to log in before you can comment on or make changes to this bug.