Closed Bug 799161 Opened 7 years ago Closed 7 years ago

Improve performance of activities registration at startup.

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: fabrice, Assigned: airpingu)

References

Details

(Whiteboard: [fast:3s])

Attachments

(1 file, 1 obsolete file)

Registering activities at startup takes a long time because we use a code path that creates a read/write indexedDB transaction for each registration.

With gaia we end up launching 15 to 20 transactions in parallel and that hurts.

Instead, we should make https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm#89 take an array of activities to register and do only one transaction.

Gene, can you take this? It would be great if we could have timings with/without this change.
Pleasure to take this! :) I'll try to improve this by following Fabrice's plan and come back with some experimental results.
Assignee: nobody → clian
Version: unspecified → Trunk
Blocks: slim-fast
Before cleaning up my patch, I'd like to share some quick experimental numbers by calculating the time diff between the start and the end of activities registration. Eventually, we could have around 3(s) improvement.

  Multiple transactions (before): 9.524(s)
  Single transaction (after):     6.551(s)

Noticeably, within the 6.551(s), almost 90% is processing _readManifests(), which reads the manifest lists of all apps (manifest.webapp). It seems this file-IO part is inevitable in any way?
Gene, that looks great!
And yes, we need to read manifests to register activities.

I *think* we could optimize further and only do this on first run (or after an update) since the activities persist in an indexedDB database, but we'd still need to register the system messages for these activities. Feel free to do this here in a Part 2 patch, or to file a follow up.
I found a potential issue in activities mechanism. Fire bug 801573. I'd prefer fixing that first since lots of codes are conflicted with that one.
Attached patch Patch (obsolete) — Splinter Review
Hi Fabrice,

(sorry for bothering you to review so often)

The patch has been done, which is summarized as below:

1. Now the "Activities:Register:OK" message carries an *array* of activities to be registered.

2. Provide a new |_registerActivitiesForApps()| which eats an *array* of |{manifest: foo, app: bar}|, which will be called by the original |_registerActivities()| with an one-element array.

3. s/_registerActivitiesForEntryPoint/_createActivitiesToRegister because we will buffer all the activities and send them at one shot.

4. Another benefit is we don't need the following mechanism anymore because all the activities are sent at one time.

  this.activitiesToRegister = 0;
  this.activitiesRegistered = 0;
  this.allActivitiesSent = false;

5. I think we don't need to return anything when the apps' registration is done:

  mm.sendAsyncMessage("Activities:Register:OK", null);

6. I did the same thing for unregistering activities.
Attachment #672195 - Flags: review?(fabrice)
blocking-basecamp: --- → ?
Fabrice said we can cut startup by about 3 seconds if we take this so let's block on it.
blocking-basecamp: ? → +
Priority: -- → P2
Comment on attachment 672195 [details] [diff] [review]
Patch

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

That looks great! r=me

::: dom/apps/src/Webapps.jsm
@@ +346,3 @@
>  
> +      for (let entryPoint in manifest.entry_points) {
> +        activitiesToRegister.push.apply(

Nit: I know that you're doing that to keep lines short, but I really dislike these |funcName(| alone on the line. I'd like to have the first parameter on the same line.
Attachment #672195 - Flags: review?(fabrice) → review+
Attached patch Patch 1.1Splinter Review
Addressing comment 7. r=fabrice.

Thanks Fabrice for the review!
Attachment #672195 - Attachment is obsolete: true
Attachment #672709 - Flags: review+
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Gene, that looks great!
> And yes, we need to read manifests to register activities.

It seems we still need to read manifests to register system messages, so I'm afraid reading manifests is inevitable for now.

> 
> I *think* we could optimize further and only do this on first run (or after
> an update) since the activities persist in an indexedDB database, but we'd
> still need to register the system messages for these activities. Feel free
> to do this here in a Part 2 patch, or to file a follow up.

This is a good point that we don't need to register activities again for the non-first run. However, since we only have single transaction now, I'm afraid this wouldn't save much time, though. Anyway, fire Bug 802994. ;)
https://hg.mozilla.org/mozilla-central/rev/fa53bf0326fb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This is marked blocking-basecamp+ but does not apply cleanly to Aurora. Please post a branch-specific patch for landing.
Hi Ryan,

We need to check in Bug 801573 to Aurora first, which needs to be marked as bb+ as well. Is this the info you want?
Yep, that's fine. Just put checkin-needed on this bug when it's OK to land.
Depends on: 801573
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.