Closed Bug 1333962 Opened 6 years ago Closed 6 years ago

Label the vsync runnable


(Core :: DOM: Content Processes, defect)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: billm, Assigned: billm)



(Whiteboard: [QDL][BACKLOG][GFX])


(1 file)

This is a tricky one since it may affect multiple DocGroups and since it may not be clear which tab it applies to until it's dispatched (I think it's whatever tab happens to be in the foreground).
Blocks: gfx-labeling
Yeah, those are equally tricky as the actual vsync runnable, except the one
in nsRefreshDriver::Thaw(), which can easily use a tabgroup, or even docgroup from GetPresContext()->GetDocument().
Blocks: 1342878
Whiteboard: [QDL][BACKLOG][GFX]
As this is identified as one of the top two to label, is there more direction we can get to start us off?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bugs)
Hi Bill,
The attached link [1] was vsync code flow from sending vsync event message in Background thread to ticking each tab in content process. In it, there are two places Gecko do dispatching.

1. Dispatching in PBackGround MessageChannel. VsyncParent calls DispatchVsyncEvent in PBackground thread to send message to Content Process.
   In Content Process, VsyncChild::RecvNotify() will be called to notify. Gecko doesn't have clear view which tab this message intends apply throughout this.
2. When VsyncChild::RecvNotify() was invoked in Content Process, it then calls TickRefreshDriver in RefreshDriverVsyncObserver() to tick by 2 steps.
   2.1 Dispatching to call TickRefreshDriver to fallback to normal priority if Gecko found the LastTickDuration(mBlockUntil) is above to TimeStamp RecvNotify() passed in.
   2.2 If not, directly calling TickRefreshDriver to tick.
   In my understanding, there is only one RefreshDriverVsyncObserver per Content Process. Based on this, Gecko can't labeling multiple tabs in this situation. 

May I have your comment to mention about what part of dispatching you are working on for labeling? It may helps to sync on your page and go on next move. Really thanks

I'm still not sure what the best way forward is for this bug. There are a couple ways to do it, and I would like to get more experience with the Quantum DOM scheduler before deciding how to fix it. For now you ignore this bug. I'll ask for help if I need it.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bugs)
Flags: needinfo?(wmccloskey)
I'm going to close this out. I think the best way to handle this is in the scheduler, not through labeling.
Closed: 6 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → INVALID
Well, there's still work to be done here, so I might as well re-open this.
Component: DOM → DOM: Content Processes
Resolution: INVALID → ---
Attached patch patchSplinter Review
This patch adds a new interface, nsILabelableRunnable. Runnables can implement it if they have complex labeling requirements. I'm hoping that vsync is the only place where we have to use it.

The way this will work is that vsync runnables will be enqueued in the high priority queue. When we want to run them, we'll QI them to nsILabelableRunnable and call GetCurrentSchedulerGroups. They'll return a list of SchedulerGroups that could be affected by the runnable. The vsync runnables will be prevented from running until all of those SchedulerGroups have finished whatever they're doing.
Attachment #8899017 - Flags: review?(kchen)
Comment on attachment 8899017 [details] [diff] [review]

Review of attachment 8899017 [details] [diff] [review]:

Overall looks good to me and some nits.

I assume the scheduler will call the new interface every time when it attempts to run the runnable? Each time the returned group might be different and the scheduler will wait until all the groups have finished their tasks? Will it be possible to create a priority inversion that dependent groups block the high priority queue forever?

::: xpcom/threads/nsILabelableRunnable.h
@@ +30,5 @@
> +// preferable to label a runnable when it is dispatched since that gives the
> +// scheduler more flexibility and will improve performance.
> +//
> +// To use this interface, QI a runnable to nsILabelableRunnable and then call
> +// GetCurrentSchedulerGroups.

The comment here could be more explicit about what will be returned through aGroups. The prose above about vsync can be easily overlooked. What's the bool return value for?

@@ +36,5 @@
> +{
> +public:
> +
> +  virtual bool GetCurrentSchedulerGroups(nsTArray<RefPtr<mozilla::SchedulerGroup>>& aGroups) = 0;

Could we call this GetApplicableSchedulerGroups()?
Attachment #8899017 - Flags: review?(kchen) → review+
Pushed by
Add nsILabelableRunnable to label runnables like vsync (r=kanru)
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.