1054638 - PBackground in workers is slow

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: speed.patch

In attach a simple mochitest based on BroadcastChannel API (bug 966439).
80% of the times, from |SendNotify| and |RecvNotify| we have more than 5 secs.

This happens just in workers. On main-thread, everything is quite fast (<100ms)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Just to clarify, this is doing:

 1) worker thread
 2) to PBackground worker thread in same process
 3) back to same worker thread in (1)

And the delay is going from (2) to (3), right?

Do you get the same timing if you run your test with --e10s?  What if you disable the code that makes workers run at a low priority (or use a chrome worker to avoid low priority)?

Comment 2

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

Are we missing to notify a monitor somewhere or what? Then when something else happens, we end up
processing the queued messages.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Try https://hg.mozilla.org/projects/jamun/rev/5da0c3cf07d8

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> Try https://hg.mozilla.org/projects/jamun/rev/5da0c3cf07d8

Hrm, you're not doing anything with blobs in that test. Maybe not relevant then.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: speed.patch

Same problem with ChromeWorkers. ~5 seconds

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

A profile of this would be very useful.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

For what its worth, I am seeing my prototype cache built on top of PBackground running slow about 50% of the time.  Seems similar amounts of delays going parent->child as reported here.

Comment 8

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Are we missing to notify a monitor somewhere or what? Then when something
> else happens, we end up
> processing the queued messages.

It seems something like this because the timing follows this pattern: quick answer < 200ms, or in 5 seconds. Nothing in between.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

To answer my previous question, this appears to effect both e10s and non-e10s.  In my particular test case I hit it more frequently with e10s enabled.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I'll look into this.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This may end up being bug 1034309

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

After investigating a bit, I don't think this is bug 1034309.  The worker actually only uses MessagePumpForNonMainThreads briefly to startup.  After that it spins the event queue in WorkerPrivate::DoRunLoop().

Part of WorkerPrivate::DoRunLoop() blocks on WaitForWorkerEvents() so that the main thread can execute a memory reporter for the worker.  This is likely what is slowing us down.  We don't notify mCondVar when an IPC message is received.

Let me see if I can come up with a patch for this.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1054638_lock_and_wakeup.patch

This somewhat crude patch fixes the issue in my local testing.  I believe Ben would prefer to go a different route, though.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8476281 [details] [diff] [review]
bug1054638_lock_and_wakeup.patch

The alternative approach Ben wanted me to try was getting the EventTarget from WorkerPrivate() and then having WorkerThread::Dispatch() call EventTarget::Dispatch().

Unfortunately this creates a cycle call graph:

  WorkerEventTarget::Dispatch()
    calls
  WorkerThread::Dispatch()
    calls
  WorkerEventTarget::Dispatch()

If this was only used on a single thread we could easily break it with a static variable, but unfortunately we must support these calls on any thread.  This means a ThreadLocal variable is needed to break the cycle.

It seems to me adding a ThreadLocal is greater complexity and heavier weight than adding a mutex as this current patch does.

Therefore I'm flagging this for review.  I'm open to other solutions or even the ThreadLocal if people still think that's better.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Another possibility might be to add WorkerThread::DispatchFromEventTarget() which would then avoid calling back into the even target.

The main difficulty here is that the WorkerThread type is private to RuntimeService.cpp right now.  Also, RuntimeService.h include WorkerPrivate.h, so simply moving WorkerThread to RuntimeService.h would not work since it would create a circular include dependency.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Wake up worker thread on Dispatch(). (v1)

Small change to avoid double-notifying the condition variable.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1054638_use_eventtarget.patch

Here is a version of the event target approach that avoids the cyclic call cycle.  Unfortunately it still suffers from a lot of recursive locking and does not work.

For example, running my CacheStorage tests:

16 INFO ###!!! ERROR: Potential deadlock detected:
17 INFO === Cyclical dependency starts at
18 INFO --- Mutex : WorkerPrivateParent::EventTarget::mMutex (currently acquired)
19 INFO  calling context
20 INFO   [stack trace unavailable]
21 INFO === Cycle completed at
22 INFO --- Mutex : WorkerPrivateParent::EventTarget::mMutex (currently acquired)
23 INFO  calling context
24 INFO   [stack trace unavailable]
25 INFO ###!!! Deadlock may happen NOW!
26 INFO [38176] ###!!! ASSERTION: Potential deadlock detected:
27 INFO Cyclical dependency starts at
28 INFO Mutex : WorkerPrivateParent::EventTarget::mMutex (currently acquired)
29 INFO Cycle completed at
30 INFO Mutex : WorkerPrivateParent::EventTarget::mMutex (currently acquired)
31 INFO ###!!! Deadlock may happen NOW!

Ben, I probably need your help to get this approach working.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8478448 [details] [diff] [review]
Wake up worker thread on Dispatch(). (v1)

I'll take a look here. Thanks!

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Move WorkerThread into its own file, v1

This just moves WorkerThread to its own file, no functional changes.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Notify the worker event loop when an XPCOM event is received, v1

This is about the safest thing I could come up with.

Basically we rely on higher-level logic. The thread shouldn't receive any events except when the worker is active.

There's fallback code for if somehow we race horribly and we receive an event while we're trying to unset the worker, but it shouldn't ever fire.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Move WorkerThread into its own file, v1

Oops, patch now includes the new files.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 1: Move WorkerThread into its own file, v2

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 2: Notify the worker event loop when an XPCOM event is received, v2

Ok, these are the correct patches. Somehow I qref'd the old ones at the wrong time.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8533431 [details] [diff] [review]
Part 2: Notify the worker event loop when an XPCOM event is received, v2

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

::: dom/workers/WorkerPrivate.cpp
@@ +2173,5 @@
> +    } else {
> +      rv = self->mThread->Dispatch(WorkerThreadFriendKey(), aRunnable);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }

Move if blah return blah out of the if?

::: dom/workers/WorkerThread.cpp
@@ +127,5 @@
> +
> +      MOZ_ASSERT(mWorkerPrivate);
> +      MOZ_ASSERT(!mAcceptingNonWorkerRunnables);
> +      MOZ_ASSERT(!mOtherThreadDispatchingViaEventTarget,
> +                 "XPCOM Dispatch hapening at the same time our thread is being "

happening

@@ +228,4 @@
>    }
>  #endif
>  
> +  WorkerPrivate* workerPrivate;

Initialize this to nullptr.

@@ +247,5 @@
> +      // to unset mWorkerPrivate while we're using it.
> +      mOtherThreadDispatchingViaEventTarget = true;
> +    } else {
> +      workerPrivate = nullptr;
> +    }

Then you don't need this branch.

@@ +290,5 @@
>    return NS_OK;
>  }
>  
> +uint32_t
> +WorkerThread::RecursionDepth(const WorkerThreadFriendKey& /* aKey */) const

Nothing calls this.  Remove it.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb081b8d3700
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c28614d1f47

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0708941a3c48

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd522fc83b5

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/bb081b8d3700
https://hg.mozilla.org/mozilla-central/rev/1c28614d1f47
https://hg.mozilla.org/mozilla-central/rev/0708941a3c48
https://hg.mozilla.org/mozilla-central/rev/5fd522fc83b5