Intent to Implement & Ship: Firefox Screenshots

[Cory Price]

This is a notice of intent to implement the Firefox Screenshots system add-on in desktop Firefox.

Links:

- Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1351424
- Repo: https://github.com/mozilla-services/screenshots/issues
- Wiki: https://wiki.mozilla.org/Firefox/Screenshots
- IRC: #screenshots
- Project deliverables / tracking: https://docs.google.com/spreadsheets/d/1axvrNaDkdA0KaadntiZH3F_rsAew72V4ZhBLqCADUpU/edit#gid=0 (note the spreadsheet is only accessible to Mozilla employees)

Overview:

Firefox Screenshots (previously called Page Shot in Test Pilot) integrates screenshot and share-to-web in Firefox with powerful search features to find past shots.

It will be shipped as a Go Faster system add-on, and also live in the Firefox tree.  Periodic releases will be imported into the tree.

Firefox Screenshots will be an Embedded WebExtension, with most of the functionality implemented in a WebExtension and a bootstrap.js wrapper to read Telemetry preferences, manage migration from the Test Pilot Page Shot add-on, and to allow users to disable the WebExtension.

Errors that occur in the Screenshots code will be reported to Mozilla’s Sentry instance.  Sentry is a server application specifically for collecting and reporting on exceptions. Behavioral events from the add-on are submitted to the Screenshots server. Both kinds of reporting are disabled if Telemetry is disabled.

Architectural documentation is in docs/ and addon/ in the repository.

For the purposes of Sentry submission we include the Raven library in the WebExtension <https://github.com/getsentry/raven-js>.  No other external libraries are used.

Timeline:

We are currently targeting Firefox 54, with the hopes of landing a fully functional MVP in advance of Firefox 54 Beta, by April 18th, 2017.

Team:

Ian Bicking: Engineering
Jared Hirsch: Engineering
Danny Coates: Engineering
Wil Clouser: Engineering Management
John Gruen: Product Management
Cory Price: Program Management
Michelle Heubusch: Content Strategy
Peter DeHaan: QA
Morpheus Chen: UX
Fang Shih: UX
And more: https://github.com/mozilla-services/screenshots/blob/master/CONTRIBUTORS.md

--

Cory Price

/ckprice

[Ehsan Akhgari]

Hi Cory,

It's exciting to see that this feature is moving on to be released!

Has there been any performance testing on Firefox with the screenshots extension installed?  I'd be curious to know some details about how this new feature impacts the performance of Firefox, especially since this is targeting to be released in Firefox 54, skipping the Nightly channel that is now 55, and WebExtension add-ons do not yet run in their own dedicated process, so I expect that the majority of the code for this add-on will run in the parent process, where performance issues can cause UI jank.  As part of the Quantum Flow project, we are working very hard to improve the performance of Firefox, and a large part of this effort is focused on improving the performance of the browser front-end, and part of this effort needs to be ensuring that new front-end features not regressing the performance before the upcoming Firefox 57 release.  Based on that, I'd be curious to know more about:

• Are there some profile links showing what the startup/shutdown of the browser looks like with this extension installed?
• Are there some profile links showing what the impact of the extension to the browser's idle time looks like?  Does the extension need to run code in the background, either lazily or periodically for certain tasks?
• Are there some profile links showing the performance of the extension as the screenshot feature itself is being used?
• Does the extension need to do I/O?  How does it achieve that?  If it uses sqlite, has there been an analysis on the sqlite query performance?  If it uses other types of storage, has there been analysis on the performance of other such mechanisms?
• Does the extension need to do expensive image manipulation on the main thread?
• Are there any other details which would be interesting to know from a performance perspective?

Thanks,

Ehsan

_______________________________________________
firefox-dev mailing list
firef...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev

--

Ehsan

[Mark Banner]

On 03/04/2017 16:45, Ehsan Akhgari wrote:

• Are there some profile links showing what the startup/shutdown of the browser looks like with this extension installed?

I'll let Ian answer most of this. What I can say that has definitely been done are Talos runs which showed no regressions.

https://github.com/mozilla-services/screenshots/issues/2317

Mark

[Ehsan Akhgari]

Talos should be thought of more as a smoke detector for this kind of feature.  That it has not shown any regressions should be a sign that the building isn't on fire, not necessarily that the feature is ready to be shipped.  :-)

Unfortunately at this time we have no automated tools that can analyze the impact of a few front-end feature without a lot of manual performance investigation.

Cheers,

--

Ehsan

[Dave Townsend]

Do we have good documentation on how to do the manual performance investigation and a plan for adding the automated testing we need to avoid it?

[Ehsan Akhgari]

The basics are pretty simple: go to https://perf-html.io, install the profiler add-on and start profiling using the instructions on that page!  We don't unfortunately have a lot of great documentation at this point due to the lack of bandwidth to produce it.  Sorry about that.  We're trying our best to move fast on all fronts here but we're really starved for choosing what to work on.  :-(

Last Friday at the Quantum Flow work week here in Toronto, I gave an informal talk about how to use the Gecko Profiler since people were interested in seeing how I use it.  I didn't have any slides or any specific profiling scenario prepared so I decided to profile Firefox live and delve into a couple of scenarios in the parent and content processes.  The presentation is being uploaded right now and will be available at this link once finished: <https://air.mozilla.org/gecko-profiler-introduction/>.  I hope this will be a good starting point.  If people have questions about how to use the profiler and how to read the profiles, I and others hanging out in #flow would be happy to help answer questions!

 

and a plan for adding the automated testing we need to avoid it?

Automated testing for this is easier said than done...  :/  In general building a tool that can analyze an arbitrary feature without knowing anything about it in advance is a research problem.  The reason tools like Talos aren't a suitable measurement utility here is that they have no tests that would directly measure anything that this feature would directly impact, and most of their tests are probably too noisy for any potential impact that the feature _could_ be introducing to show up in any significant way in the test output numbers.  This excludes tests like Ts, in theory, but in practice even those tests have been less helpful in preventing startup regressions than I would really like...  Why that is the case, I'm not really sure.  It could be due to issues in Talos, or in how we use it, or both.  Again, using Ts as an example, we have gotten more than twice as bad over the past year on this Ts Windows 32-bit e10s PGO measurement <https://treeherder.mozilla.org/perf.html#/graphs?timerange=31536000&series=%5Bmozilla-central,e394aab72917d169024558cbab33eb4e7e9504e1,1,1%5D>, so perhaps there is just a death by a thousand cuts effect at play here.

But for the time being, I think we shouldn't bet the farm on just running tests on Talos, especially for features that don't even get the regular Talos testing that normal features developed on m-c get.

--

Ehsan

[Cory Price]

Hi Ehsan,

Thanks for your questions! We've put some answers together below (although Ian may have more to add here when he returns Wednesday).

> Are there some profile links showing what the impact of the extension to the browser's idle time looks like?  Does the extension need to run code in the background, either lazily or periodically for certain tasks?

No code runs in the background.

> Are there some profile links showing the performance of the extension as the screenshot feature itself is being used?

This performance hasn't been graphed.  We should minimize any slowdown here, but also recognize that it would be from the direct action of a user and so could be expected. Thanks for the perf-html information. We've created an issue to create one of these profiles (https://github.com/mozilla-services/screenshots/issues/2554), would you have any resources to help us get going here? And/or is there more documentation aside from your response in this thread?

We've also created an issue to record how long it takes to process an image (https://github.com/mozilla-services/screenshots/issues/2555).

> Does the extension need to do I/O?  How does it achieve that?  If it uses sqlite, has there been an analysis on the sqlite query performance?  If it uses other types of storage, has there been analysis on the performance of other such mechanisms?

We don't use sqlite, we do use `browser.storage.local` for a small amount of account data.

> Does the extension need to do expensive image manipulation on the main thread?

No.

> Are there any other details which would be interesting to know from a performance perspective?

This is a relatively simple extension that is not in the primary UI flow. It doesn't do anything until the user specifically engages with it.

--

Cory Price

/ckprice

[Ehsan Akhgari]

On Mon, Apr 3, 2017 at 1:20 PM, Cory Price <cpr...@mozilla.com> wrote:

Hi Ehsan,

Thanks for your questions! We've put some answers together below (although Ian may have more to add here when he returns Wednesday).

> Are there some profile links showing what the impact of the extension to the browser's idle time looks like?  Does the extension need to run code in the background, either lazily or periodically for certain tasks?

No code runs in the background.

Great!

Does the add-on inject a frame script into every page?  Or only into the pages that the user wants to take a screenshot from?

 

> Are there some profile links showing the performance of the extension as the screenshot feature itself is being used?

This performance hasn't been graphed.  We should minimize any slowdown here, but also recognize that it would be from the direct action of a user and so could be expected. Thanks for the perf-html information. We've created an issue to create one of these profiles (https://github.com/mozilla-services/screenshots/issues/2554), would you have any resources to help us get going here? And/or is there more documentation aside from your response in this thread?

What kind of resources did you have in mind?  I personally have very limited time these days and a ton of things to investigate, but I looked a bit at the code and found a few things that are probably worth paying more attention to, which I commented on in the issue above.  Someone needs to poke into this using a profiler to discover more.  :-)  But thankfully it seems that the add-on code base is relatively small so it should be fairly simple for someone familiar with the code to come up with an effective approach of examining all of the use cases where the add-on can be expected to run code, run them under the profiler and see what comes out of it.  The profiler UI makes it very easy to search for things, so for example you can easily search for the names of the functions used in the add-on code to see where they show up in the profile and how much time they spend, etc.

If you needed help reading profiles, I'd be happy to take a look if you post some links here.

 

We've also created an issue to record how long it takes to process an image (https://github.com/mozilla-services/screenshots/issues/2555).

> Does the extension need to do I/O?  How does it achieve that?  If it uses sqlite, has there been an analysis on the sqlite query performance?  If it uses other types of storage, has there been analysis on the performance of other such mechanisms?

We don't use sqlite, we do use `browser.storage.local` for a small amount of account data.

> Does the extension need to do expensive image manipulation on the main thread?

No.

> Are there any other details which would be interesting to know from a performance perspective?

This is a relatively simple extension that is not in the primary UI flow. It doesn't do anything until the user specifically engages with it.

These all sound good!

Thanks again,

Ehsan

 

On Mon, Apr 3, 2017 at 9:47 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

On Mon, Apr 3, 2017 at 12:19 PM, Dave Townsend <dtow...@mozilla.com> wrote:

On Mon, Apr 3, 2017 at 9:16 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

On Mon, Apr 3, 2017 at 12:11 PM, Mark Banner <mba...@mozilla.com> wrote:

On 03/04/2017 16:45, Ehsan Akhgari wrote:

• Are there some profile links showing what the startup/shutdown of the browser looks like with this extension installed?

I'll let Ian answer most of this. What I can say that has definitely been done are Talos runs which showed no regressions.

https://github.com/mozilla-services/screenshots/issues/2317

Talos should be thought of more as a smoke detector for this kind of feature.  That it has not shown any regressions should be a sign that the building isn't on fire, not necessarily that the feature is ready to be shipped.  :-)

Unfortunately at this time we have no automated tools that can analyze the impact of a few front-end feature without a lot of manual performance investigation.

Do we have good documentation on how to do the manual performance investigation

The basics are pretty simple: go to https://perf-html.io, install the profiler add-on and start profiling using the instructions on that page!  We don't unfortunately have a lot of great documentation at this point due to the lack of bandwidth to produce it.  Sorry about that.  We're trying our best to move fast on all fronts here but we're really starved for choosing what to work on.  :-(

Last Friday at the Quantum Flow work week here in Toronto, I gave an informal talk about how to use the Gecko Profiler since people were interested in seeing how I use it.  I didn't have any slides or any specific profiling scenario prepared so I decided to profile Firefox live and delve into a couple of scenarios in the parent and content processes.  The presentation is being uploaded right now and will be available at this link once finished: <https://air.mozilla.org/gecko-profiler-introduction/>.  I hope this will be a good starting point.  If people have questions about how to use the profiler and how to read the profiles, I and others hanging out in #flow would be happy to help answer questions!

 

and a plan for adding the automated testing we need to avoid it?

Automated testing for this is easier said than done...  :/  In general building a tool that can analyze an arbitrary feature without knowing anything about it in advance is a research problem.  The reason tools like Talos aren't a suitable measurement utility here is that they have no tests that would directly measure anything that this feature would directly impact, and most of their tests are probably too noisy for any potential impact that the feature _could_ be introducing to show up in any significant way in the test output numbers.  This excludes tests like Ts, in theory, but in practice even those tests have been less helpful in preventing startup regressions than I would really like...  Why that is the case, I'm not really sure.  It could be due to issues in Talos, or in how we use it, or both.  Again, using Ts as an example, we have gotten more than twice as bad over the past year on this Ts Windows 32-bit e10s PGO measurement <https://treeherder.mozilla.org/perf.html#/graphs?timerange=31536000&series=%5Bmozilla-central,e394aab72917d169024558cbab33eb4e7e9504e1,1,1%5D>, so perhaps there is just a death by a thousand cuts effect at play here.

But for the time being, I think we shouldn't bet the farm on just running tests on Talos, especially for features that don't even get the regular Talos testing that normal features developed on m-c get.

--

Ehsan

--

Cory Price

/ckprice

--

Ehsan

[Cory Price]

On Mon, Apr 3, 2017 at 12:32 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

On Mon, Apr 3, 2017 at 1:20 PM, Cory Price <cpr...@mozilla.com> wrote:

Hi Ehsan,

Thanks for your questions! We've put some answers together below (although Ian may have more to add here when he returns Wednesday).

> Are there some profile links showing what the impact of the extension to the browser's idle time looks like?  Does the extension need to run code in the background, either lazily or periodically for certain tasks?

No code runs in the background.

Great!

Does the add-on inject a frame script into every page?  Or only into the pages that the user wants to take a screenshot from?

Not on every page, so the latter. Ref: https://github.com/mozilla-services/screenshots/issues/2554#issuecomment-291263613

 

 

> Are there some profile links showing the performance of the extension as the screenshot feature itself is being used?

This performance hasn't been graphed.  We should minimize any slowdown here, but also recognize that it would be from the direct action of a user and so could be expected. Thanks for the perf-html information. We've created an issue to create one of these profiles (https://github.com/mozilla-services/screenshots/issues/2554), would you have any resources to help us get going here? And/or is there more documentation aside from your response in this thread?

What kind of resources did you have in mind?  I personally have very limited time these days and a ton of things to investigate, but I looked a bit at the code and found a few things that are probably worth paying more attention to, which I commented on in the issue above. 

Someone needs to poke into this using a profiler to discover more.  :-) 

Thanks for commenting! A resource to "poke at this using a profiler to discover more" as noted below would be great to have. Our backlog is full, so identifying anything severe should happen soon (this week?), and someone familiar with the tools and severity of items would be extremely helpful.

If there's documentation on the "performance review" process and ownership I'd love to subscribe so I'm not behind on the next thing.
 

--

Cory Price

/ckprice

[Ehsan Akhgari]

On Tue, Apr 4, 2017 at 4:56 PM, Kris Maglione <kmag...@mozilla.com> wrote:

On Mon, Apr 03, 2017 at 11:45:22AM -0400, Ehsan Akhgari wrote:

Has there been any performance testing on Firefox with the screenshots extension installed?

There's a known startup performance regression when any WebExtension is installed, since we don't load most of the WebExtension modules until they're needed. This is less of an issue now that bugs 1350522 and 1344590 are fixed, and will be less still once bug 1317697 is fixed and extensions are loaded out of process by default.

It's probably worth noting that this isn't a per-extension regression. We pay the price for the first extension that we load. The subsequent ones we basically get for free. So once OOP extensions are enabled by default, we actually run less risk of startup perf or UI jank regressions for features implemented as extensions than we would otherwise.

This does mean that the first extension that gets shipped to all users by default like this will cause this regression to happen, right?

 

WebExtension add-ons do not yet run in their own dedicated process, so I expect that the majority of the code for this add-on will run in the parent process, where performance issues can cause UI jank.

The majority of the code should run in a content script, which would usually run in the content process if e10s is enabled. The code that runs in the parent process is mostly pretty trivial.

  - Does the extension need to do I/O?  How does it achieve that?  If it

  uses sqlite, has there been an analysis on the sqlite query performance?
  If it uses other types of storage, has there been analysis on the
  performance of other such mechanisms?

Note: I'm currently pushing for the extension to switch its storage backend to IndexedDB, for the sake of transactions, which is backed by SQLite. But its storage usage is minimal, and primarily just for credential storage.

Yeah, after looking at the extension code, it doesn't look like there is anything to worry about storage wise.

--

Ehsan

[Ehsan Akhgari]

On Mon, Apr 3, 2017 at 5:12 PM, Cory Price <cpr...@mozilla.com> wrote:

On Mon, Apr 3, 2017 at 12:32 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

On Mon, Apr 3, 2017 at 1:20 PM, Cory Price <cpr...@mozilla.com> wrote:

Hi Ehsan,

Thanks for your questions! We've put some answers together below (although Ian may have more to add here when he returns Wednesday).

> Are there some profile links showing what the impact of the extension to the browser's idle time looks like?  Does the extension need to run code in the background, either lazily or periodically for certain tasks?

No code runs in the background.

Great!

Does the add-on inject a frame script into every page?  Or only into the pages that the user wants to take a screenshot from?

Not on every page, so the latter. Ref: https://github.com/mozilla-services/screenshots/issues/2554#issuecomment-291263613

Sounds great!

 

 

> Are there some profile links showing the performance of the extension as the screenshot feature itself is being used?

This performance hasn't been graphed.  We should minimize any slowdown here, but also recognize that it would be from the direct action of a user and so could be expected. Thanks for the perf-html information. We've created an issue to create one of these profiles (https://github.com/mozilla-services/screenshots/issues/2554), would you have any resources to help us get going here? And/or is there more documentation aside from your response in this thread?

What kind of resources did you have in mind?  I personally have very limited time these days and a ton of things to investigate, but I looked a bit at the code and found a few things that are probably worth paying more attention to, which I commented on in the issue above. 

Someone needs to poke into this using a profiler to discover more.  :-) 

Thanks for commenting! A resource to "poke at this using a profiler to discover more" as noted below would be great to have. Our backlog is full, so identifying anything severe should happen soon (this week?), and someone familiar with the tools and severity of items would be extremely helpful.

If there's documentation on the "performance review" process and ownership I'd love to subscribe so I'm not behind on the next thing.

We don't have a great set of resources and documentation here I'm afraid.  :-(  For the time being, I'm happy to fill in instead of that by helping answer questions etc as needed.  Please let me know if you need help, ping me on IRC, or email, bugzilla, etc.

FWIW many people are asking for resources to help them with profiling and analyzing performance issues, so we should definitely work on improving documentation, and create some tutorials, etc.  I'm hoping to see that story improved soon.  Sorry for the current state of things...

--

Ehsan

[Dave Townsend]

On Tue, Apr 4, 2017 at 2:44 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:

On Tue, Apr 4, 2017 at 4:56 PM, Kris Maglione <kmag...@mozilla.com> wrote:

On Mon, Apr 03, 2017 at 11:45:22AM -0400, Ehsan Akhgari wrote:

Has there been any performance testing on Firefox with the screenshots extension installed?

There's a known startup performance regression when any WebExtension is installed, since we don't load most of the WebExtension modules until they're needed. This is less of an issue now that bugs 1350522 and 1344590 are fixed, and will be less still once bug 1317697 is fixed and extensions are loaded out of process by default.

It's probably worth noting that this isn't a per-extension regression. We pay the price for the first extension that we load. The subsequent ones we basically get for free. So once OOP extensions are enabled by default, we actually run less risk of startup perf or UI jank regressions for features implemented as extensions than we would otherwise.

This does mean that the first extension that gets shipped to all users by default like this will cause this regression to happen, right?

Have we measured this regression? Do we have a bug on file for it? Should this block shipping screenshots?

[Ehsan Akhgari]

The measurement at least certainly should.  It's hard to say more without knowing how severe the actual regression is.

--

Ehsan

[Kris Maglione]

On Wed, Apr 05, 2017 at 10:05:15AM -0700, Dave Townsend wrote:
>On Tue, Apr 4, 2017 at 2:44 PM, Ehsan Akhgari <ehsan....@gmail.com>
>wrote:

>> This does mean that the first extension that gets shipped to all users by
>> default like this will cause this regression to happen, right?
>
>Have we measured this regression? Do we have a bug on file for it?

Yes. See bug 1317681. I believe it's somewhere around 45ms at this point.

[Ehsan Akhgari]

On Wed, Apr 5, 2017 at 1:11 PM, Kris Maglione <kmag...@mozilla.com> wrote:

On Wed, Apr 05, 2017 at 10:05:15AM -0700, Dave Townsend wrote:

On Tue, Apr 4, 2017 at 2:44 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

This does mean that the first extension that gets shipped to all users by
default like this will cause this regression to happen, right?

Have we measured this regression? Do we have a bug on file for it?

Yes. See bug 1317681. I believe it's somewhere around 45ms at this point.

Ugh.  :-(  I took a quick look at the dependency chain there, and the existing fixes don't jump at me as stuff that we'd necessarily want to backport to 54, is my impression there correct?  If yes, that would be a problem since Cory's original email up-thread here mentioned 54 as the target release for this feature...

--

Ehsan

[Dave Townsend]

It looks like here are only two of the dependant bugs that aren't already fixed in 54, bug 1344590 and bug 1350522, both of which were added as dependencies after the most recent numbers were measured.

[Ian Bicking]

On Wed, Apr 5, 2017 at 12:11 PM, Kris Maglione <kmag...@mozilla.com> wrote:

On Wed, Apr 05, 2017 at 10:05:15AM -0700, Dave Townsend wrote:

On Tue, Apr 4, 2017 at 2:44 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

This does mean that the first extension that gets shipped to all users by
default like this will cause this regression to happen, right?

Have we measured this regression? Do we have a bug on file for it?

Yes. See bug 1317681. I believe it's somewhere around 45ms at this point.

We have an explicit call to webExtension.startup() in our bootstrap.js – would the performance hit be when the startup method is called, or before then (e.g., when the webExtension object is created)?  Just curious if we could accomplish anything by deferring the startup.

The other possible regression that Screenshots could introduce is that it adds a content script to all screenshots.firefox.com pages.  The content script itself isn't a concern IMHO (it's just part of the performance of screenshot pages) but as a result it does activate the the WebExtension matching code that decides whether to add the content script to a given page.

 

[Kris Maglione]

Correct, I definitely wouldn't want to uplift those changes to 54. So we
should probably specifically profile the startup impact of the Screenshots
add-on in 54, and if we find it to be problematic, we can defer startup of the
WebExtension portion until after first paint.

[Kris Maglione]

On Wed, Apr 05, 2017 at 12:28:16PM -0500, Ian Bicking wrote:
>On Wed, Apr 5, 2017 at 12:11 PM, Kris Maglione <kmag...@mozilla.com>
>wrote:
>
>> On Wed, Apr 05, 2017 at 10:05:15AM -0700, Dave Townsend wrote:
>>
>>> On Tue, Apr 4, 2017 at 2:44 PM, Ehsan Akhgari <ehsan....@gmail.com>
>>> wrote:
>>>
>>>> This does mean that the first extension that gets shipped to all users by
>>>> default like this will cause this regression to happen, right?
>>>>
>>>
>>> Have we measured this regression? Do we have a bug on file for it?
>>>
>>
>> Yes. See bug 1317681. I believe it's somewhere around 45ms at this point.
>
>
>We have an explicit call to webExtension.startup() in our bootstrap.js –
>would the performance hit be when the startup method is called, or before
>then (e.g., when the webExtension object is created)? Just curious if we
>could accomplish anything by deferring the startup.

There should be no significant performance hit until after startup() is
called. Hm. Or, at least, there wouldn't if LegacyExtensionUtils didn't
eagerly import a couple of other WebExtension modules. We should be able to
fix that in an upliftable way, though. XPIProvider really doesn't need to load
that module until the startup() method is called.

>The other possible regression that Screenshots could introduce is that it
>adds a content script to all screenshots.firefox.com pages. The content
>script itself isn't a concern IMHO (it's just part of the performance of
>screenshot pages) but as a result it does activate the the WebExtension
>matching code that decides whether to add the content script to a given
>page.

You're right that that introduces a small amount of additional overhead, but
at this point it's pretty reasonable. I'm planning to rewrite most of that
matching code in C++ soon, at which point the overhead probably won't be
measurable.

[Jared Hirsch]

We have an explicit call to webExtension.startup() in our bootstrap.js – would the performance hit be when the startup method is called, or before then (e.g., when the webExtension object is created)?  Just curious if we could accomplish anything by deferring the startup.

There should be no significant performance hit until after startup() is called. Hm. Or, at least, there wouldn't if LegacyExtensionUtils didn't eagerly import a couple of other WebExtension modules. We should be able to fix that in an upliftable way, though. XPIProvider really doesn't need to load that module until the startup() method is called.

Ah, but note that our bootstrap.js directly Cu.import()s LegacyExtensionUtils. Would it help to lazily import that module instead? LegacyExtensionUtils isn't called inside bootstrap until after startup(), so, maybe? https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L61
 

[Dave Townsend]

On Wed, Apr 5, 2017 at 12:17 PM, Jared Hirsch <6a...@mozilla.com> wrote:

We have an explicit call to webExtension.startup() in our bootstrap.js – would the performance hit be when the startup method is called, or before then (e.g., when the webExtension object is created)?  Just curious if we could accomplish anything by deferring the startup.

There should be no significant performance hit until after startup() is called. Hm. Or, at least, there wouldn't if LegacyExtensionUtils didn't eagerly import a couple of other WebExtension modules. We should be able to fix that in an upliftable way, though. XPIProvider really doesn't need to load that module until the startup() method is called.

Ah, but note that our bootstrap.js directly Cu.import()s LegacyExtensionUtils. Would it help to lazily import that module instead? LegacyExtensionUtils isn't called inside bootstrap until after startup(), so, maybe? https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L61

Sounds like we should go ahead and defer the startup of the webextension in bootstrap.js and that should take care of any performance problems during startup.

Are there any other performance concerns we should be investigating more?

[Michael de Boer]

On 5 Apr 2017, at 21:17, Jared Hirsch <6a...@mozilla.com> wrote:

We have an explicit call to webExtension.startup() in our bootstrap.js – would the performance hit be when the startup method is called, or before then (e.g., when the webExtension object is created)?  Just curious if we could accomplish anything by deferring the startup.

There should be no significant performance hit until after startup() is called. Hm. Or, at least, there wouldn't if LegacyExtensionUtils didn't eagerly import a couple of other WebExtension modules. We should be able to fix that in an upliftable way, though. XPIProvider really doesn't need to load that module until the startup() method is called.

Ah, but note that our bootstrap.js directly Cu.import()s LegacyExtensionUtils. Would it help to lazily import that module instead? LegacyExtensionUtils isn't called inside bootstrap until after startup(), so, maybe? https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L61

Absolutely! These are cheap wins (as in: small change, big impact). I’d do the same thing for AddonManager.jsm (which is a pretty large module to read, parse & evaluate) and Console.jsm.

The only added JSM would be XPCOMUtils.jsm, but that one’s already required by many other modules, thus won’t add compartments/ overhead.

Hope this helps,

Mike.

[Dave Townsend]

AddonManager is already loaded by this point so importing it should cost nothing. I expect we load Console.jsm elsewhere during startup already too.

[Kris Maglione]

On Wed, Apr 05, 2017 at 12:57:39PM -0700, Dave Townsend wrote:
>AddonManager is already loaded by this point so importing it should cost
>nothing. I expect we load Console.jsm elsewhere during startup already too.

Note that calling Cu.import isn't free, even if the module is already loaded.
defineLazyModule getter isn't free easier, but it's much cheaper than calling
into XPConnect.

[Jared Hirsch]

Talked this over with Kris and Dave in IRC just now. I'm going to change bootstrap.js to:
(1) lazy load all the Cu.imports;
(2) delay WebExtension startup until the "sessionstore-windows-restored" event.

Hopefully this will address the startup performance regression.

[Wil Clouser]

Thanks for the discussion and drilling down to actionable items.  This is filed as https://github.com/mozilla-services/screenshots/issues/2575

Cheers,

Wil

[Wil Clouser]

That issue is closed (Thanks Jared) and I don't know of any other asks here so I'm assuming we're good to go.  Thanks everyone.

Wil

[Ehsan Akhgari]

Hi Wil and the team,

Thanks for the investigations so far!

Did the work in https://github.com/mozilla-services/screenshots/issues/2554 lead to uncovering any issues?

Thanks,

Ehsan

--

Ehsan

[Jared Hirsch]

Hi Ehsan,

I've been hunting down test failures, haven't had a chance to run the perf tests yet.

Let's coordinate in the bug :+1:

Cheers,

Jared