Bug 1321812 (Labeling)

DocGroup/TabGroup/SystemGroup labeling

NEW
Unassigned

Status

()

Core
DOM
5 months ago
16 hours ago

People

(Reporter: mccr8, Unassigned)

Tracking

(Depends on: 24 bugs, Blocks: 1 bug, {meta})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Comment hidden (empty)
Blocks: 1302270
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.
Oops, I forgot: https://wiki.mozilla.org/Quantum/DOM is the "canonical document" for this project.
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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Depends on: 1328517
Depends on: 1328526
Depends on: 1330512

Updated

3 months ago
Depends on: 1314833

Updated

3 months ago
Depends on: 1331804

Updated

3 months ago
Depends on: 1332494
(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.
(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.
(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.
(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.
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
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?
(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!

Updated

3 months ago
Alias: Labeling

Updated

3 months ago
Depends on: 1333962

Updated

3 months ago
Depends on: 1332491

Updated

3 months ago
Depends on: 1333968

Updated

3 months ago
Depends on: 1333971

Updated

3 months ago
Depends on: 1333974

Updated

3 months ago
Depends on: 1333977

Updated

3 months ago
Depends on: 1333981

Updated

3 months ago
Depends on: 1333983

Updated

3 months ago
Depends on: 1333984
Thanks for all these feedbacks!
NI just for the question missed in comment 10.
Flags: needinfo?(wmccloskey)
Depends on: 1333997
(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.

Updated

3 months ago
Flags: needinfo?(wmccloskey)
(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! :)
Flags: needinfo?(wmccloskey)
(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!
Flags: needinfo?(wmccloskey)

Updated

3 months ago
Depends on: 1334572

Updated

3 months ago
Depends on: 1335601

Updated

3 months ago
Depends on: 1337537

Updated

3 months ago
No longer depends on: 1334572

Updated

3 months ago
Depends on: 1337575

Updated

3 months ago
Depends on: 1333333
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.
(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.
Depends on: 1338446

Updated

2 months ago
Depends on: 1339004

Updated

2 months ago
Depends on: 1339006

Updated

2 months ago
Depends on: 1339289

Updated

2 months ago
Depends on: 1339224
No longer depends on: 1338446
Depends on: 1339343
Depends on: 1339676

Updated

2 months ago
Depends on: 1339707
Depends on: 1341537
Depends on: 1341539
Depends on: 1341540

Updated

2 months ago
Depends on: 1343756
Depends on: 1345464
Depends on: 1346145
Depends on: 1347815
No longer depends on: 1347815

Updated

a month ago
Depends on: 1350787
Summary: DocGroup labeling → DocGroup/TabGroup/SystemGroup labeling

Updated

a month ago
Depends on: 1350910

Updated

a month ago
Depends on: 1350926

Updated

a month ago
Depends on: 1350933

Updated

a month ago
Depends on: 1350937
Depends on: 1350938
Depends on: 1350940
Depends on: 1350943

Updated

a month ago
Depends on: 1350949

Updated

a month ago
Depends on: 1350953
Depends on: 1350958
Depends on: 1350968
Depends on: 1351021
Depends on: 1359676
Depends on: 1359686
Depends on: 1359700
Depends on: 1359705
Depends on: 1359706
No longer depends on: 1359706
You need to log in before you can comment on or make changes to this bug.