849943 - Investigate removing the extra dispatch for child process notifications in MainThreadEventTarget

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Bug 848316 showed some fun slowness in a gaia app due to a busy event loop. The problem is made slightly worse by the fact that we do an extra NS_DispatchToMainThread from the MainThreadEventTarget helper whenever we receive a message from the parent. Removing this dispatch causes immediate assertion failures/crashes in our IPC tests so it's not a simple thing to remove. If we can remove it we should though.

Comment 1

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

Attachment: Patch

Comment 2

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

Comment on attachment 724186 [details] [diff] [review]
Patch

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

r=me with this:

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +636,2 @@
>  {
>    *aIsOnCurrentThread = NS_IsMainThread();

Just make this return true always.

::: dom/indexedDB/ipc/IndexedDBChild.cpp
@@ +390,3 @@
>    if (NS_FAILED(openHelper->Dispatch(&target))) {
>      NS_WARNING("Dispatch of IPCOpenDatabaseHelper failed!");
>      return false;

Hm, before we had to check for failure because NS_DispatchToMainThread could conceivably fail. Now we're always going to just call Run on runnables that we control, right? So we should be able to MOZ_ASSERT that they never fail rather than do release-mode checks here I think.

Comment 3

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

This was cloned, not supposed to be tef+.

Comment 4

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

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

Comment 5

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

This small change will speed up indexedDB requests a little for child processes (see bug 848316 for some context). Can we get this into b2g18?

Comment 6

[Michael Vines [:m1] [:evilmachines]]

bent - do you have a sense for how much this bug could help improve contacts app startup, say with 500 contacts?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e4be735f546

Comment 8

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #6)
> bent - do you have a sense for how much this bug could help improve contacts
> app startup, say with 500 contacts?

No, it's really impossible to say without measuring the final contacts app code (we're still trying to tweak it to make scrolling work while loading). The problem is that the speedup depends on how busy the child process' main event loop is. If the event loop is free then there will be no speedup with this patch. If the event loop is very busy then the speedup could be significant.

Comment 9

[Michael Vines [:m1] [:evilmachines]]

Ok, thanks.  tef- for now until we can determine if this will do anything meaningful in v1.0.1

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

As with comment 9 - if there's no clear win here then there's not much of a case for tracking & uplifting.  Please re-nom if there is a strong case and a good reward vs. risk.