Closed Bug 1567258 Opened 5 years ago Closed 5 years ago

Make Monitor a built-in component

Categories

(Firefox :: Firefox Monitor, task, P3)

task

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: aswan, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

We're not shipping out-of-cycle updates to Firefox Monitor, so we're paying system addon overhead for it unnecessarily. A first draft patch (that still needs some l10n work) to convert it to a built-in component is here:
https://phabricator.services.mozilla.com/D37457

Lets measure the impact on startup performance and, if it looks good, polish off that patch and get it landed.

Nihanth, we talked about this a while ago, but just want to confirm: we don't have any specific reasons to keep shipping monitor as an extension right?

Flags: needinfo?(nhnt11)
Whiteboard: [fxperf]

Here are talos comparisons for the patch mentioned above, though there's not a very strong signal:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5612defaf368e15634a9843df572d53e65b1f6f5&newProject=try&newRevision=d65ba6ebc6371ca103eabeb7940af5b73f987dda

However, we do have some evidence that this would make an impact on the first run (and on runs after upgrades)
This is a profile from a reference device that shows over 1 second spent in XPIProvider.checkForChanges():
https://perfht.ml/2Z032Yl

This function synchronously reads and parses the manifests for all built-in system addons. Monitor is one of 5 system addons that exist at the time this profile was taken, so making monitor built-in would not remove all that overhead but it would take a nice bite out of it.

I don't think we have any specific reasons to keep shipping Monitor as an extension. I believe that there are some changes to the doorhanger in the roadmap for the Monitor features in Firefox, and that might be a good opportunity to move the code into browser/base. We definitely don't have cycles to allocate for this work in 70, and probably not in 71 either unless the timeline accelerates. Thanks for filing this and for the talos comparisons + first run profile!

Flags: needinfo?(nhnt11)
Priority: -- → P3

Sounds like this is relatively straightforward and offers potential startup improvements esp. on clean profiles.

Blocks: 1546460
Whiteboard: [fxperf] → [fxperf:p2]

@aswan
Is this something you still plan to land, or should we try to find a new owner?

Flags: needinfo?(andrew.swan)

My phabricator account got disabled last week and I'm still trying to get access to it. The requested changes are quite small, if somebody else is available to finish this off, that would be great. Nihanth, any chance you can do it?

Flags: needinfo?(andrew.swan) → needinfo?(nhnt11)
Attachment #9083079 - Attachment description: Bug 1567258 Convert fxmonitor to a built-in component → Bug 1567258 - Convert fxmonitor to a built-in component
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/740a6df70b30
Convert fxmonitor to a built-in component r=nhnt11,flod
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: