Open Bug 904306 Opened 11 years ago Updated 2 years ago

Worker code is under commented

Categories

(Core :: DOM: Workers, defect, P5)

x86_64
Linux
defect

Tracking

()

People

(Reporter: nsm, Unassigned)

References

Details

dom/workers/ has significantly complex code, particularly due to all the message passing across threads, the two separate trains of thought required between a) Worker implementation (WorkerPrivate, WorkerPrivateParent, WorkerScope and co.) b) DOM facing interface ([Shared]Worker) and the APIs that are ported over to workers. The recent addition of cycle collection and upcoming addition of SharedWorkers will only increase complexity. While there are great assertion checks ensuring that it is obvious which thread functions should run on, there is no easy way to get a high level picture of things running at a time, especially ownership models and lifetimes. In addition, it would be helpful if parts that explicitly implemented the spec could point to spec sections and summarize what they were doing.
Do you have specific questions? "Write more stuff down" sounds good but isn't very actionable :-/
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1) > Do you have specific questions? "Write more stuff down" sounds good but > isn't very actionable :-/ I thought I was pretty specific in the description :) Anyway: a) The Runnable heirarchy does a lot of things, so a one liner explaining what each runnable does, who dispatches it (from which thread), who receives it (on which thread) would be great. b) There is the WorkerPrivate, but also a WorkerPrivateParent<>. I couldn't figure out why the split. c) mParentStatus is the hardest. Is it the status of the parent worker for a child worker? Because the points where it is used, there are no checks that the caller is a child worker. For a top level worker, the mParentStatus doesn't make sense, yet it is used for various status changes. d) The setTimeout/Interval implementation uses just one timer and queues. While this is obvious on reading the code, a comment will save time. e) A comment near where bindings are loaded about whether this applies to WebIDL bindings only. If XPIDL bindings can be used in workers, links to examples of how to use them or a summary. In addition not a comment in the code, but a question now that I've thought of it: How do I load JS implemented bindings into workers? Especially those that need a window object? Where will the worker implementation live if a different implementation is required? How do I specify that in Bindings.conf or in the webidl files. f) If the state machine that is the worker lifecycle status could be summarized, that'd be a huge help. The spec has no concept of Closing vs Terminating vs Killing, and the code has no concept of active vs suspended workers (that is in the spec). And as mentioned in the description, code that does very specific spec algorithms should add section comments the way the JS engine does. I'll add more as I go through the code.
Also it isn't evident that ModifyBusyCount, when passed false, will *decrease* the busy count and not leave it unchanged. Maybe change the API with an explicit enum { Increase, Decrease } ? The semantics of WorkerRunnable::{Pre,Post}Dispatch, what they should do and what the return values mean.
(In reply to Nikhil Marathe [:nsm] from comment #2) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1) > > Do you have specific questions? "Write more stuff down" sounds good but > > isn't very actionable :-/ > > I thought I was pretty specific in the description :) > Anyway: It probably helps to start by saying that every worker has the concept of a worker thread and a parent thread. The worker thread is (obviously) the thread the worker runs on. The parent thread is the thread the worker was created from. This is either the main thread (for a top-level worker created directly by a page) or another worker. In the worker code 'parent' generally refers to methods called or data accessed on the parent thread. > a) The Runnable heirarchy does a lot of things, so a one liner explaining > what each runnable does, who dispatches it (from which thread), who receives > it (on which thread) would be great. Fair enough. > b) There is the WorkerPrivate, but also a WorkerPrivateParent<>. I couldn't > figure out why the split. The methods on WorkerPrivateParent are called on the parent thread, the methods on WorkerPrivate on the worker thread. > c) mParentStatus is the hardest. Is it the status of the parent worker for a > child worker? Because the points where it is used, there are no checks that > the caller is a child worker. For a top level worker, the mParentStatus > doesn't make sense, yet it is used for various status changes. mParentStatus is the status on the parent thread. To maintain proper event loop ordering we can't just lock and fiddle with the status, we have to update the status off an event that waits in the queue so other stuff gets a chance to process. > d) The setTimeout/Interval implementation uses just one timer and queues. > While this is obvious on reading the code, a comment will save time. Yeah I'm not super familiar with this. I think it's basically the same as the main thread implementation. > e) A comment near where bindings are loaded about whether this applies to > WebIDL bindings only. If XPIDL bindings can be used in workers, links to > examples of how to use them or a summary. You can't use XPIDL bindings in workers. Those are XPConnect (and hence main-thread) only. Your only options are WebIDL bindings or the legacy hand coded JSAPI bindings. All new code should be using WebIDL. > In addition not a comment in the > code, but a question now that I've thought of it: > > How do I load JS implemented bindings into workers? Especially those that > need a window object? Where will the worker implementation live if a > different implementation is required? How do I specify that in Bindings.conf > or in the webidl files. You don't. There's no support for this. > f) If the state machine that is the worker lifecycle status could be > summarized, that'd be a huge help. The spec has no concept of Closing vs > Terminating vs Killing, and the code has no concept of active vs suspended > workers (that is in the spec). And as mentioned in the description, code > that does very specific spec algorithms should add section comments the way > the JS engine does. http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerFeature.h#13 ?
(In reply to Nikhil Marathe [:nsm] from comment #3) > Also it isn't evident that ModifyBusyCount, when passed false, will > *decrease* the busy count and not leave it unchanged. Maybe change the API > with an explicit enum { Increase, Decrease } ? http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html ;-) > The semantics of WorkerRunnable::{Pre,Post}Dispatch, what they should do and > what the return values mean. Generally you don't need to override these. Most of the overrides are stuff dealing with timers which can be dispatched from multiple threads (and which confuse the assertions).
Also sometimes Pre/PostDispatch are overridden to assert that we're on the main thread for things that end up proxying only to the main thread.
The event loop setup could probably use better documentation (e.g. what sync queues are, when to use them, what control messages are and when they run).
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.