Closed Bug 1300658 Opened 3 years ago Closed 3 years ago

throttle runnables from Worker threads to the main thread

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 10 obsolete files)

1.85 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.54 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.11 KB, patch
baku
: review+
Details | Diff | Splinter Review
7.53 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.82 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.81 KB, patch
baku
: feedback+
Details | Diff | Splinter Review
15.00 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.18 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Bug 1300118 adapts the xpcom TaskQueue class a bit to be used to prevent main thread runnable flooding.  This bug will use TaskQueue in dom/workers to prevent flooding the main thread from a worker thread.

For example, this patch will keep this worker script from killing main thread responsiveness:

  while(true) { console.log('foo'); }
Probably best not to use a WorkerPrivate* right after setting it to nullptr.
Attachment #8788300 - Attachment is obsolete: true
Rebase around changes in bug 1300118.  Also, change the API around a little bit to focus on nsIEventTarget instead of TaskQueue.
Attachment #8788295 - Attachment is obsolete: true
Attachment #8788696 - Flags: review?(amarchesini)
I removed that other assertion per the last comment.  I also tried to make the patch description text better reflect what its doing.
Attachment #8788297 - Attachment is obsolete: true
Attachment #8788749 - Flags: review?(jwwang)
Comment on attachment 8788749 [details] [diff] [review]
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang

Wrong bug...
Attachment #8788749 - Attachment is obsolete: true
Attachment #8788749 - Flags: review?(jwwang)
Attachment #8788297 - Attachment is obsolete: false
Attachment #8788297 - Flags: review?(amarchesini)
Attachment #8788298 - Flags: review?(amarchesini)
Attachment #8788439 - Flags: review?(amarchesini)
Attachment #8788301 - Flags: review?(amarchesini)
Attachment #8788297 - Flags: review?(amarchesini) → review+
Comment on attachment 8788698 [details] [diff] [review]
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

Review of attachment 8788698 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ -401,5 @@
>      mFinishedWorker->ForgetMainThreadObjects(doomed);
>  
>      RefPtr<MainThreadReleaseRunnable> runnable =
>        new MainThreadReleaseRunnable(doomed, loadGroupToCancel);
> -    if (NS_FAILED(NS_DispatchToMainThread(runnable))) {

I think this super error prone. Can we change NS_DispatchToMainThread so that we don't need to change the way we dispatch runnables to main-thread in workers?
Attachment #8788698 - Flags: review?(amarchesini)
Flags: needinfo?(bkelly)
(In reply to Andrea Marchesini [:baku] from comment #17)
> I think this super error prone. Can we change NS_DispatchToMainThread so
> that we don't need to change the way we dispatch runnables to main-thread in
> workers?

Well, I generally think that its better to be explicit about the queue we are dispatching to and not try to do magic in the NS_DispatchToMainThread().  It lets us:

1) Incrementally roll this out to parts of the tree a little bit at a time.
2) Makes it explicit at the callsite which queue or "bucket" you are using.
3) Allows us to introspect the queue or "bucket" and perform back-pressure logic specific to the caller.  For example, an nsTimeout implementation could freeze timers if its queue is backed up, etc.
4) There may be cases where we don't want to use the queue or "bucket".  That's much easier to code and understand later if we are explicitly dispatching to a different nsIEventTarget handle.
5) It might be confusing that you get this magic behavior in NS_DispatchToMainThread(), but not if you do do_GetMainThread()->Dispatch().  Doing this above the xpcom layer for now avoids this kind of confusion.

For workers we could possibly use GetCurrentThreadWorkerPrivate() in NS_DispatchToMainThread(), but I'd rather not do that to start.  I'm not sure that approach will work well in non-worker thread cases and I don't want to set the precedent for it yet.

I'd like to prove this out as a layer above xpcom first and then later pull it into our automatic dispatch routines once its stable.

Anyway, thats my view on it.  What do you think?
Flags: needinfo?(bkelly) → needinfo?(amarchesini)
This patch builds on P1 and replaces P2 to P6.  It makes NS_DispatchToMainThread() use the WorkerPrivate if we're on a worker thread.

Andrea, is this what you had in mind?

Nathan, what do you think of this?  Would you be ok with a dependency from xpcom to workers here?
Attachment #8789553 - Flags: feedback?(nfroyd)
Attachment #8789553 - Flags: feedback?(amarchesini)
Comment on attachment 8789553 [details] [diff] [review]
Make NS_DispatchToMainThread() use WorkerPrivate::DispatchToMainThread() on worker threads. r=froydnj

Review of attachment 8789553 [details] [diff] [review]:
-----------------------------------------------------------------

This does not strike me as a good idea.  I'd really like to limit the dependencies that xpcom has on other parts of the code; we have enough dependencies already.  This is also increasing the overhead for every caller of NS_DispatchToMainThread, too.

I don't understand Andrea's objection in comment 17; is the concern that people in worker code have to remember to use ->DispatchToMainThread rather than something else?
Attachment #8789553 - Flags: feedback?(nfroyd)
Depends on: 1301519
I'm able to use GetCurrentThreadWorkerPrivate() in a couple places in service workers with the patch in bug 1301519.
Attachment #8788439 - Attachment is obsolete: true
Attachment #8788439 - Flags: review?(amarchesini)
Attachment #8789580 - Flags: review?(amarchesini)
Attachment #8788696 - Flags: review?(amarchesini) → review+
Attachment #8789553 - Flags: feedback?(amarchesini) → feedback+
> I don't understand Andrea's objection in comment 17; is the concern that
> people in worker code have to remember to use ->DispatchToMainThread rather
> than something else?

Right. that is the point. but avoiding dependences seems to me a good point as well.

Workers have already a custom way to dispatch runnables. With these patches we are introducing a new way to dispatch runnables from workers to the main-thread. If some part of the code forgets to use ->DispatchToMainThread we could end up having runnables scheduled in the wrong order and this could introduce serious bugs.

But I let Nathan decide this as XPCOM owner.
Flags: needinfo?(amarchesini)
Comment on attachment 8788298 [details] [diff] [review]
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku

Review of attachment 8788298 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/Fetch.cpp
@@ +968,5 @@
> +  nsresult rv = NS_OK;
> +  if (mWorkerPrivate) {
> +    rv = mWorkerPrivate->DispatchToMainThread(r.forget());
> +  } else {
> +    rv = NS_DispatchToMainThread(r.forget());

Yeah, that is the thing I would like to avoid.
Maybe we can introduce a new DispatchToMainThread(workerPrivateOrNull, runnable) method?
Attachment #8788298 - Flags: review?(amarchesini) → review+
Attachment #8788301 - Flags: review?(amarchesini) → review+
Attachment #8789580 - Flags: review?(amarchesini) → review+
See comment 22 and 23.
Flags: needinfo?(nfroyd)
(In reply to Andrea Marchesini [:baku] from comment #22)
> > I don't understand Andrea's objection in comment 17; is the concern that
> > people in worker code have to remember to use ->DispatchToMainThread rather
> > than something else?
> 
> Right. that is the point. but avoiding dependences seems to me a good point
> as well.
> 
> Workers have already a custom way to dispatch runnables. With these patches
> we are introducing a new way to dispatch runnables from workers to the
> main-thread. If some part of the code forgets to use ->DispatchToMainThread
> we could end up having runnables scheduled in the wrong order and this could
> introduce serious bugs.

My sense--as an outside observer, not somebody who deals with this on a regular basis--is that workers already have enough special rules about how to operate that adding DispatchToMainThread almost makes things *more* regular, not less.  (We could also put in DEBUG-only assertions in NS_DispatchToMainThread for catching cases when you ought to be using ->DispatchToMainThread.)  Moving towards full-blown document-specific event queues will also bring about more of this "call special methods to dispatch runnables" behavior, so doing it in workers now doesn't seem like a bad thing.
Flags: needinfo?(nfroyd)
Comment on attachment 8788698 [details] [diff] [review]
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

I said I would reflag this last week in IRC, but forgot.  Of course, feel free to discuss options with Nathan first.
Attachment #8788698 - Flags: review?(amarchesini)
Attachment #8788698 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b67fecfa036
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/623fb54446e0
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2453a17bc6c1
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d98a586c04d
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6fb32982e7
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/980659720b86
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku
Backed out for bad bug number in commit message:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fd5c60f2b6
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56496fad6494
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e921d61f036
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9d70b040a2
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/39fbc61739ef
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c74062a27462
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf3a60640cf
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku
Fairly certain this test races removing this file with stopping the use of the file in the os.file chrome worker:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_downloads_search.js#98
Flags: needinfo?(bkelly)
Blocks: QuantumDOM
Comment on attachment 8790568 [details] [diff] [review]
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

Andrew, this patch fixes a small race in the cleanup of this test.  It calls downloadDir.remove(), but its possible for components in the system or OS to still be holding the file open.  On linux and mac this is fine, but on windows .remove() throws an error if the file is still open.

This patch loops on removing the file until its gone or we give up after 25 times.  This is how we handle the problem in other places in the xpcshell test runner:

https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#475
Attachment #8790568 - Flags: review?(aswan)
Comment on attachment 8790568 [details] [diff] [review]
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

Review of attachment 8790568 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, windows.  I don't really understand the relationship to the bug this is linked to (I assume your changes for that bug made this start failing regularly?  but why?).  There is very similar code to this in a bunch of the other test_ext_downloads_*.js test files, off the top of my head I can't think of a reason why this one is special and it is failing while the others aren't.
But in any case, I suspect you don't want to spend a bunch of time plumbing the depths of these particular tests, the actual fix seems reasonable enough, r+.

::: toolkit/components/extensions/test/xpcshell/head.js
@@ +97,5 @@
> +      } catch (e) {
> +        // ignore
> +      }
> +      if (!dir.exists() || count >= 25) {
> +        return resolve();

If this hasn't succeeded after 25 tries, I think it should reject instead of resolving without actually removing the directory.
Attachment #8790568 - Flags: review?(aswan) → review+
(In reply to Andrew Swan [:aswan] from comment #37)
> Ugh, windows.  I don't really understand the relationship to the bug this is
> linked to (I assume your changes for that bug made this start failing
> regularly?  but why?).  There is very similar code to this in a bunch of the
> other test_ext_downloads_*.js test files, off the top of my head I can't
> think of a reason why this one is special and it is failing while the others
> aren't.
> But in any case, I suspect you don't want to spend a bunch of time plumbing
> the depths of these particular tests, the actual fix seems reasonable
> enough, r+.

This patch can slightly delay the async os.file operations since they use a worker thread.  (This patch throttles worker threads.)  I think we were just getting lucky in the past.  Its possible we could wait for all the async file operations to complete, but I don't know the downloads.jsm well enough to trace it all through.  Since this is just test cleanup I think this is a reasonable fix.

> > +      if (!dir.exists() || count >= 25) {
> > +        return resolve();
> 
> If this hasn't succeeded after 25 tries, I think it should reject instead of
> resolving without actually removing the directory.

We don't treat it as an error in the other place I linked to in comment 36, but I can do it.
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62775b8b0e7b
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/23830bda70ee
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66ad87e9f77
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/466a5fb382ed
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ee5e6a9c9d
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3440336fa8
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7275e98b63b
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan
Duplicate of this bug: 1326016
Duplicate of this bug: 1179871
You need to log in before you can comment on or make changes to this bug.