1333962 - Label the vsync runnable

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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).

Comment 1

[Tommy Kuo (away forever...)]

There are some events related to vsync in layout/base/nsRefreshDriver.cpp[1].

[1]: http://searchfox.org/mozilla-central/search?q=NS_DispatchTo(Main%7CCurrent)Thread&case=false&regexp=true&path=layout%2Fbase%2FnsRefreshDriver.cpp

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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().

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

As this is identified as one of the top two to label, is there more direction we can get to start us off?

Comment 4

[Vincent Liu[:vliu]]

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

[1]:https://docs.google.com/presentation/d/1P-o4Vy8UIaQKfAONJ1-P0AoLOCqs_xZOT_e8SwG13M8/edit#slide=id.p

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm going to close this out. I think the best way to handle this is in the scheduler, not through labeling.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, there's still work to be done here, so I might as well re-open this.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

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.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8899017 [details] [diff] [review]
patch

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:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ILABELABLERUNNABLE_IID);
> +
> +  virtual bool GetCurrentSchedulerGroups(nsTArray<RefPtr<mozilla::SchedulerGroup>>& aGroups) = 0;

Could we call this GetApplicableSchedulerGroups()?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1557d8ea43
Add nsILabelableRunnable to label runnables like vsync (r=kanru)

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f1557d8ea43