1321812 - (Labeling) [meta] DocGroup/TabGroup/SystemGroup labeling

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P5)

Comment 1

[Nick Alexander :nalexander [he/him]]

It's worth linking to some of the context around this project.

billm's blog post includes a sketch of DocGroup (and TabGroup) labeling: https://billmccloskey.wordpress.com/2016/10/27/mozillas-quantum-project/.  Start here for the very high-level view.

The initial post to dev-platform includes an overview, call to action, additional links, and some discussion about impacts on other parts of the project, including devtools: https://groups.google.com/d/msg/mozilla.dev.platform/Zb7clqn_WAU/NaeL6u-1CwAJ.  Follow the links here for more of the details.

Comment 2

[Nick Alexander :nalexander [he/him]]

Oops, I forgot: https://wiki.mozilla.org/Quantum/DOM is the "canonical document" for this project.

Comment 3

[Nick Alexander :nalexander [he/him]]

billm, others: I have a question about https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#15019.  It looks like this only handles DocGroups, but if a DocGroup isn't known, aren't we supposed to fall back to a TabGroup?  It appears this code doesn't do that, but I see no examples in the tree that try to fall back to a TabGroup.  Is a DocGroup always known?  Is my understanding of the fall back idea incorrect?

This might be profitably explained in the Wiki page.

Comment 4

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

The SetEventTarget function in this patch has an example of falling back to the TabGroup:
https://reviewboard.mozilla.org/r/94090/diff/2#index_header

Generally I don't think it's worth falling back to the TabGroup just because the DocGroup is null. A null DocGroup is a corner case for when we don't have a document, or the document doesn't have principals yet. Usually that's pretty uncommon. The networking case is different because we top-level loads never have a document, and that's certainly a common occurrence.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #4)
> Generally I don't think it's worth falling back to the TabGroup just because
> the DocGroup is null. A null DocGroup is a corner case for when we don't
> have a document, or the document doesn't have principals yet. Usually that's
> pretty uncommon. The networking case is different because we top-level loads
> never have a document, and that's certainly a common occurrence.
Then, holding a *nullable* DocGroup reference for off-main-thread to main-thread dispatching seems not a good recommendation in https://wiki.mozilla.org/Quantum/DOM#Example
In general, to the API users, it's more clear to hold an *non-nullable* EventTarget from nsDocument/nsGlobalWindow which returns the default one if DocGroup-version is not available.
For naming runnable, it can still be done via nsINamed interface.

Comment 6

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> (In reply to Bill McCloskey (:billm) from comment #4)
> > Generally I don't think it's worth falling back to the TabGroup just because
> > the DocGroup is null. A null DocGroup is a corner case for when we don't
> > have a document, or the document doesn't have principals yet. Usually that's
> > pretty uncommon. The networking case is different because we top-level loads
> > never have a document, and that's certainly a common occurrence.
> Then, holding a *nullable* DocGroup reference for off-main-thread to
> main-thread dispatching seems not a good recommendation in
> https://wiki.mozilla.org/Quantum/DOM#Example
> In general, to the API users, it's more clear to hold an *non-nullable*
> EventTarget from nsDocument/nsGlobalWindow which returns the default one if
> DocGroup-version is not available.

I see your point. Clients don't want to have to worry about whether the DocGroup is null. The problem is that we don't have a notion of a "default DocGroup" right now.

Maybe we should have a GetDispatcher() method that would return either a DocGroup (if there is one) or some kind of DefaultDispatcher singleton that would just dispatch to the main thread? People could use that if they don't know whether the DocGroup is null or not.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #6) 
> I see your point. Clients don't want to have to worry about whether the
> DocGroup is null. The problem is that we don't have a notion of a "default
> DocGroup" right now.
I think the API user don't really care too much about whether it's a default DocGroup or not.
They just need an instance for dispatching (which implicitly handles DocGroup-Labeling) without too much worry. :p
> Maybe we should have a GetDispatcher() method that would return either a
> DocGroup (if there is one) or some kind of DefaultDispatcher singleton that
> would just dispatch to the main thread? People could use that if they don't
> know whether the DocGroup is null or not.

I thought that the subclass of nsIGlobalObject (nsDocument, nsGlobalWindow) which overrides EventTargetFor() and AbstractMainThreadFor() have done a good job for this in a similar way, unless we really need a Dispatcher instance.

IMHO, the only benefit I see to hold a dispatcher instance instead of an EventTarget is to have the runnable named when Dispatch() is called, but this can be done easily by calling SetName() or overriding the GetName() on most of the Runnables.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> IMHO, the only benefit I see to hold a dispatcher instance instead of an
> EventTarget is to have the runnable named when Dispatch() is called, but
> this can be done easily by calling SetName() or overriding the GetName() on
> most of the Runnables.
The other options is to have Name parameters in those NewRunnableMethods defined in http://searchfox.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h instead if the dispatched Runnable was not created by defining a class that inherits Runnable.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Here are some feedback and questions after having 1st brownbag for these APIs to gfx team in TPE Office today:
1. How to specify EventTarget to the TimerCallback for DocGroup Labeling?
   - This can be done by nsTimer::SetTarget(nsIEventTarget*). Please correct me if it's wrong.
2. Will |aCategory| be used for prioritizing in the future? Or, it won't and it just a marker for further analysis. If it will, how can developers ensure that the ordering won't be a problem if tasks are labeled in different categories in current developing stage?
3. How to start the the labeling?
   - Review every line includes keywords of "Dispatch" and "nsITimer". 
   - Did I miss anything? Any better idea?
4. *Important*: Only the runnables to the main thread of the content process has to be labeled.

BrownBag slides for reference:
https://www.google.com/url?q=https%3A%2F%2Fwww.icloud.com%2Fkeynote%2F0hmu8sUZRot_9XJACva_hdnKA%23DocGroup%255FLabeling%255Fin%255FQuantum%255FDOM&sa=D&ust=1485337842756000&usg=AFQjCNGenIQ8lbsBV3ovOoxvwBgNcKKpdg

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

5. What's the impact if the caller of NS_DispatchToMainThread() that doesn't touch any content JS was not replaced with SystemGroup APIs provided in bug 1332494?

Comment 11

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> I thought that the subclass of nsIGlobalObject (nsDocument, nsGlobalWindow)
> which overrides EventTargetFor() and AbstractMainThreadFor() have done a
> good job for this in a similar way, unless we really need a Dispatcher
> instance.
> 
> IMHO, the only benefit I see to hold a dispatcher instance instead of an
> EventTarget is to have the runnable named when Dispatch() is called, but
> this can be done easily by calling SetName() or overriding the GetName() on
> most of the Runnables.

Yes, that's true. Using an nsIEventTarget would work as well. I can update the example.

> The other options is to have Name parameters in those NewRunnableMethods defined in
> http://searchfox.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h instead if
> the dispatched Runnable was not created by defining a class that inherits Runnable.

That makes sense. Feel free to file a bug.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9)
> Here are some feedback and questions after having 1st brownbag for these
> APIs to gfx team in TPE Office today:
> 1. How to specify EventTarget to the TimerCallback for DocGroup Labeling?
>    - This can be done by nsTimer::SetTarget(nsIEventTarget*). Please correct
> me if it's wrong.

Yes, that's correct. We do that here, for example:
http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/dom/xhr/XMLHttpRequestMainThread.cpp#2980

> 2. Will |aCategory| be used for prioritizing in the future? Or, it won't and
> it just a marker for further analysis. If it will, how can developers ensure
> that the ordering won't be a problem if tasks are labeled in different
> categories in current developing stage?

It may be used for scheduling in the future. Generally, anything that's not marked as Other should come from some outside source (usually IPC, but also from a timer). In that case, we have a lot of freedom about when to schedule it. So there shouldn't be too many concerns about breaking things as long as people follow that rule.

> 3. How to start the the labeling?
>    - Review every line includes keywords of "Dispatch" and "nsITimer". 
>    - Did I miss anything? Any better idea?

I think it's best to start with the list in bug 1331804. The patches in that bug will generate telemetry that will allow us to see which runnables run most often. We should start with those.

However, if someone is an expert on a particular sub-system and wants to just fix that code, then looking for calls to NS_DispatchTo{Main,Current}Thread or to nsIThread::Dispatch is a good way to go.

> 4. *Important*: Only the runnables to the main thread of the content process
> has to be labeled.

Correct.

> BrownBag slides for reference:
> https://www.google.com/url?q=https%3A%2F%2Fwww.icloud.
> com%2Fkeynote%2F0hmu8sUZRot_9XJACva_hdnKA%23DocGroup%255FLabeling%255Fin%255F
> Quantum%255FDOM&sa=D&ust=1485337842756000&usg=AFQjCNGenIQ8lbsBV3ovOoxvwBgNcKK
> pdg

I read this over and it looks really nice. Thanks!

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Thanks for all these feedbacks!
NI just for the question missed in comment 10.

Comment 13

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

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> 5. What's the impact if the caller of NS_DispatchToMainThread() that doesn't
> touch any content JS was not replaced with SystemGroup APIs provided in bug
> 1332494?

Say the event queue contains these runnables: [F1, B1, F2, B2, F3]. Assume that the F runnables are for the foreground tab and the B runnables are for a background tab. Then we could run F1, F2, and F3 before we run any of the B runnables. That's good.

However, say the event queue contains: [F1, B1, F2, B2, X, F3]. Assume that X is an unlabeled runnable dispatched using NS_DispatchToMainThread. We can run F1 and F2. However, before we can run F3, we need to run B1, B2, and X. That's bad. Why is that?

Well, X could be an F runnable (so maybe X = F2.5). If we ran F1, F2, F3 before the other runnables, then we would run F's events out of order (since F3 would run before F2.5). Therefore we need to run X before we run F3.

However, X might also be a B runnable (so X = B3). Because of that, we need to run B1 and B2 before we run X. In total, B1, B2, and X must run before F3.

So if we find an unlabeled event X in the queue, we need to run all the events before it before we can run any event after it. However, if we label X as a SystemGroup runnable, then we know it has no effect on the F or the B tabs. So it's fine to run it whenever we want. Therefore we could run F1, F2, F3 before any other runnables.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #13)
> So if we find an unlabeled event X in the queue, we need to run all the
> events before it before we can run any event after it. However, if we label
> X as a SystemGroup runnable, then we know it has no effect on the F or the B
> tabs. So it's fine to run it whenever we want. Therefore we could run F1,
> F2, F3 before any other runnables.

Oops, I thought SystemGroup API was just a marker from the patch in bug 1332494 and the unlabeled runnables shall behave in the same way as SystemGroup runnables as you explained, because all the runnables shall already be identified and labeled before the scheduler is ready to be enabled but it seems incorrect.
I guess the reason is that this allows us to enable the scheduler earlier once the implementation is done before having all runnable labeled. Is this correct?

BTW, there is another question about labeling main thread runnables only in content process:
Will these Dispatcher/SystemGroup APIs help to fallback to NS_DispatchToMainThread() internally if the runnable was run in parent by checking XRE_IsParentProcess()?
The reason to ask is that, for the implementations that will be available in both parent and child processes(like PBackgrounChilds or XPCOM modules available in both processes), the labeling change could be tedious.

Hope it won't be too annoying to you to keep asking you questions. :p
Your answers are really helpful for me to clarify the concern before the next brown bag! :)

Comment 15

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

(In reply to Bevis Tseng[:bevistseng][:btseng] (Chinese New Year until 2/6) from comment #14)
> (In reply to Bill McCloskey (:billm) from comment #13)
> > So if we find an unlabeled event X in the queue, we need to run all the
> > events before it before we can run any event after it. However, if we label
> > X as a SystemGroup runnable, then we know it has no effect on the F or the B
> > tabs. So it's fine to run it whenever we want. Therefore we could run F1,
> > F2, F3 before any other runnables.
> 
> Oops, I thought SystemGroup API was just a marker from the patch in bug
> 1332494 and the unlabeled runnables shall behave in the same way as
> SystemGroup runnables as you explained, because all the runnables shall
> already be identified and labeled before the scheduler is ready to be
> enabled but it seems incorrect.
> I guess the reason is that this allows us to enable the scheduler earlier
> once the implementation is done before having all runnable labeled. Is this
> correct?

Yes, that's correct.

> BTW, there is another question about labeling main thread runnables only in
> content process:
> Will these Dispatcher/SystemGroup APIs help to fallback to
> NS_DispatchToMainThread() internally if the runnable was run in parent by
> checking XRE_IsParentProcess()?
> The reason to ask is that, for the implementations that will be available in
> both parent and child processes(like PBackgrounChilds or XPCOM modules
> available in both processes), the labeling change could be tedious.

Yes, we'll fall back to NS_DispatchToMainThread in the parent process. There's no need to label there (although it doesn't hurt).

> Hope it won't be too annoying to you to keep asking you questions. :p
> Your answers are really helpful for me to clarify the concern before the
> next brown bag! :)

Thanks for all the questions!

Comment 16

[Cameron McCormack (:heycam)]

How do we handle NS_ProxyRelease calls and uses of nsMainThreadPtrHolder?  The events we dispatch to destroy objects using these probably can't be observed by content, although I couldn't be sure.

Comment 17

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

(In reply to Cameron McCormack (:heycam) from comment #16)
> How do we handle NS_ProxyRelease calls and uses of nsMainThreadPtrHolder? 
> The events we dispatch to destroy objects using these probably can't be
> observed by content, although I couldn't be sure.

See bug 1333974.

Comment 18

[Massimo Fidanza]

No maintainer for this?

Comment 19

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

There isn't particular needs for this right now given bug 1451850, which will fix many of the issue Quantum DOM was meant to fix, but bug 1451850 will fix them in a different way.

Comment 20

[Massimo Fidanza]

Why wiki page was not updated since 17 May 2017
https://wiki.mozilla.org/Quantum/DOM

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Massimo Fidanza from comment #20)

Why wiki page was not updated since 17 May 2017
https://wiki.mozilla.org/Quantum/DOM

I'm not familiar with the specifics, but the runnable prioritization work did not end up improving performance, so we've been removing it. As Olli said in comment 19, people have been working on other approaches to prioritizing running things from different pages.

At this point, I think the labeling is only used for improving error messages for leak checking runnables. I'm going to mark this bug as fixed, as lots of stuff has been labeled.