Closed
Bug 799161
Opened 12 years ago
Closed 12 years ago
Improve performance of activities registration at startup.
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
People
(Reporter: fabrice, Assigned: airpingu)
References
Details
(Whiteboard: [fast:3s])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Updated•12 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 2•12 years ago
|
||
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?
Reporter | ||
Comment 3•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [fast:3s]
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 6•12 years ago
|
||
Fabrice said we can cut startup by about 3 seconds if we take this so let's block on it.
blocking-basecamp: ? → +
Updated•12 years ago
|
Priority: -- → P2
Reporter | ||
Comment 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
Addressing comment 7. r=fabrice.
Thanks Fabrice for the review!
Attachment #672195 -
Attachment is obsolete: true
Attachment #672709 -
Flags: review+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
(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. ;)
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 11•12 years ago
|
||
Keywords: checkin-needed
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 13•12 years ago
|
||
This is marked blocking-basecamp+ but does not apply cleanly to Aurora. Please post a branch-specific patch for landing.
Assignee | ||
Comment 14•12 years ago
|
||
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?
Comment 15•12 years ago
|
||
Yep, that's fine. Just put checkin-needed on this bug when it's OK to land.
Comment 16•12 years ago
|
||
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•