Closed Bug 849943 Opened 7 years ago Closed 7 years ago

Investigate removing the extra dispatch for child process notifications in MainThreadEventTarget

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g -
Tracking Status
b2g18 - ---

People

(Reporter: bent.mozilla, Assigned: khuey)

References

Details

Attachments

(1 file)

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.
Attachment #724186 - Attachment is patch: true
Attachment #724186 - Flags: review?(bent.mozilla)
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.
Attachment #724186 - Flags: review?(bent.mozilla) → review+
Assignee: nobody → khuey
This was cloned, not supposed to be tef+.
blocking-b2g: tef+ → ---
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?
blocking-b2g: --- → tef?
tracking-b2g18: --- → ?
bent - do you have a sense for how much this bug could help improve contacts app startup, say with 500 contacts?
Flags: needinfo?(bent.mozilla)
https://hg.mozilla.org/mozilla-central/rev/1e4be735f546
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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.
Flags: needinfo?(bent.mozilla)
Ok, thanks.  tef- for now until we can determine if this will do anything meaningful in v1.0.1
blocking-b2g: tef? → -
Blocks: 848316
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.
You need to log in before you can comment on or make changes to this bug.