1350472 - [meta] Reduce the number of jsms loaded in content processes

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Andrew McCreight [:mccr8]]

Every jsm we load has a compartment that uses 25kb to 30kb, based on what I see in about:memory. That cost is paid per content process, so with e10s multi that can add up. I'm going to look into what we can do to reduce the number of jsms we have loaded at any given time. I'll start with simpler things and then build towards fancier things as the need arises.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Log JS stacks for imports. Not for landing.

This is a crude patch I have that dumps the JS stack any time you import a file. I also have a post processing script that parses out who is loading what. I've used this to try to reduce the number of jsms that are loaded at startup.

Comment 2

[Andrew McCreight [:mccr8]]

The most basic improvement is removing imports that aren't used. Potentially we can detect these statically. Bill suggested I could try using Esprima for this. Ideally, we'd have a linter kind of tool to detect these.

Another set of improvements is importing more JSMs lazily, using the existing methods for that. It might be possible to detect these dynamically. In other words, if you load a JSM, and it loads some other JSM in the process, but does not use anything in that other JSM during loading, maybe it should be lazy.

There are also some basic extensions of the lazy importing that would be nice. We load various JSMs for the JSON viewer, WebRTC and PDF.js, but it seems like we shouldn't need to unless we're actually loading content of that type.

Bill also suggested that we should be able to unload JSMs automatically when they aren't used for a while. This would require that the JSM have code to store its state, if any, so that it could be restored later as needed.

Comment 3

[Gabor Krizsanits (INACTIVE)]

This is great, I wanted to work on this as well but haven't got the time for it yet... maybe once e10s-multi is landed. I've did some logging as well about the various frame scripts, process scripts and JSM's we load at process startup [1] but loading things more lazily turned out to be a bit more work than I expected.

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Another set of improvements is importing more JSMs lazily, using the
> existing methods for that. It might be possible to detect these dynamically.
> In other words, if you load a JSM, and it loads some other JSM in the
> process, but does not use anything in that other JSM during loading, maybe
> it should be lazy.

There has been quite some work invested in loading things lazily and most of the low hanging fruit are already taken there. One thing I think we should focus on is breaking huge modules into two parts (at least). (1) stub that attaches some dummy observers and listeners, and load the rest of the stuff only when needed (2) the heavy part that we instantiate only at the first event or whatever it triggers it. The good thing is that once we have this pattern, we can start looking into unloading the heavy bits once they are no longer needed and just keep the dummy listeners. If you were already referring to something like this by "lazy" and not something like |loader.lazyGetter| and the like, then just ignore my comment :)

[1]: https://bug635044.bugzilla.mozilla.org/show_bug.cgi?id=1317304#c1

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> but loading things more lazily turned out to be a bit
> more work than I expected.

Yeah, just in the first thing I looked at, skipping lazy loading of osfile stuff in telemetry, I've uncovered at least 3 bugs.

> If you were already referring to something like this by "lazy" and not something like
> |loader.lazyGetter| and the like, then just ignore my comment :)

That's what I meant, but I guess I didn't really explain it well. Thanks for the information on what you've looked at.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: notes on startup jsms

Here's a rough analysis of all of the .jsms we load when starting the browser and then loading Hacker News and what we might need to avoid loading them.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: analyzer for output from stack patch

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: notes on startup jsms (updated)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8854213 [details]
notes on startup jsms (updated)

Have we considered writing a talos test that checks the number of jsms that are loaded?

Some other comments on some things I just happened to spot on the latest list (by no means a thorough check!):

>May be fixable:
>  chrome://pocket/content/AboutPocket.jsm

I'm confused, actually, it doesn't look like the panel in which we load the pocket iframe is remote, so that would allow removing this (ie all it does is register about:pocket-signup and about:pocket-saved, which are never used in the content process AIUI...) - but it probably *should* ideally be remote, except that would probably require its own set of work.

Regardless, the only reason this seems to need a jsm right now is to facilitate loading its own content pages. It could (and should) just use other URLs for that, they don't need to be about: URLs at all, I think. Per bug 1236755 they shouldn't be chrome URLs, but resource: URLs should be unprivileged enough for what we want here, maybe/hopefully?

Also, I vaguely recall there's now code to just open a tab to do sign-up, so maybe the sign-up flow could be switched to that entirely, and we can stop using a frame for the 'saved' bits, and then we can forget about this whole sorry saga.

>Telemetry: ???

>  resource://gre/modules/DeferredTask.jsm
This I would expect other places to use, too, but maybe other consumers use lazy module getters?

>  resource://gre/modules/debug.js (only imported from Telemetry)
Yuck. A "jsm" which has a .js extension and has only 1 function in it? It looks like only TelemetrySession requires it from telemetry, might be simplest to just shift those consumers to something else. It seems we use it more extensively in the parent process, though I'm unconvinced it's really all that useful.

Another option would be moving it to AppConstants or Services.jsm and also ifdef'ing it into a no-op on release builds (which is what it currently checks at runtime, which seems particularly unhappy from a perf perspective).


>  resource://gre/modules/Log.jsm (only imported from Telemetry)
>  resource://gre/modules/PromiseUtils.jsm (only imported from Telemetry)
These 2 are also surprising to me in that I'd expect other things to use them.

>  resource://gre/modules/ServiceRequest.jsm from resource://gre/modules/TelemetrySend.jsm:26
>  resource://gre/modules/TelemetryController.jsm
>  resource://gre/modules/TelemetrySend.jsm
>  resource://gre/modules/TelemetrySession.jsm
>  resource://gre/modules/TelemetryTimestamps.jsm
>  resource://gre/modules/TelemetryUtils.jsm

I'm fairly sure not all of these should be necessary in the content processes. Folks in #telemetry will know more.

>Spell checker: ???
>  resource://gre/modules/InlineSpellCheckerContent.jsm from chrome://browser/content/content.js:16
>  resource://gre/modules/InlineSpellChecker.jsm from chrome://browser/content/content.js:15
>  resource://gre/modules/InlineSpellChecker.jsm from resource://gre/modules/InlineSpellCheckerContent.jsm:11
This looks like an easy fix - we should only need these once you context-click on a spellcheckable area. We can remove the InlineSpellChecker.jsm import from content.js and make the other import lazy.

>Probably not easily fixable: (mostly things that need to analyze each page as it loads)
>  resource://gre/modules/ReaderMode.jsm
I wonder if we could make this more lightweight, though. We could potentially factor out the 'isprobablyreaderable' stuff instead of loading the entire jsm and whatever it depends on, until the user actually activates reader mode.

>  resource:///modules/ContentLinkHandler.jsm
If it gains us something significant, it looks like the event listeners for meta elements could live in the main content process script (the result of the content-observers change), and be attached/removed when globals are created (for which there are observer notifications, I think).

>  resource:///modules/Feeds.jsm
All the message listeners are for the parent process, I think, so only isValidFeed is really needed in the content process, so we could maybe move that elsewhere. That also cuts out the useless event listeners. isValidFeed is only called from content.js, so we could just move it there or to ContentLinkHandler.jsm if we keep that.

>  resource:///modules/FormSubmitObserver.jsm

This is already a dep of this bug for this, bug 1250487. I think we could lazily import it after the first invalidformsubmit observer notification, which are pretty rare, so might be worth doing - that or just stick the entire thing in the process script.

>Generic utilities probably not worth looking into:
>  AppConstants.jsm, BrowserUtils.jsm, Console.jsm, Preferences.jsm,
>  PrivateBrowsingUtils.jsm, Promise.jsm, Task.jsm, Timer.jsm

Moving to async/await should get rid of Promise.jsm and Task.jsm. I actually am not a big fan of Preferences.jsm but I don't know that there's consensus on that. Now that the pref service itself supports default values I think it's lost some of its usefulness. Console.jsm should ideally be lazy-getter'd from everywhere. I don't think we log stuff to it as a general rule except in, well, exceptional situations, so maybe that'll help. Unless we have code that uses it for tracing behind a pref or something...

Comment 9

[Marco Castelluccio [:marco]]

There are other ideas of components that could be removed/merged blocking bug 986323 (e.g. bug 986501, bug 986503, bug 988096).

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gijs from comment #8)
> >  resource://gre/modules/debug.js (only imported from Telemetry)
> Yuck. A "jsm" which has a .js extension and has only 1 function in it?

This looks pretty historic, filed bug 1354036.

> >  resource://gre/modules/ServiceRequest.jsm from resource://gre/modules/TelemetrySend.jsm:26
> >  resource://gre/modules/TelemetryController.jsm
> >  resource://gre/modules/TelemetrySend.jsm
> >  resource://gre/modules/TelemetrySession.jsm
> >  resource://gre/modules/TelemetryTimestamps.jsm
> >  resource://gre/modules/TelemetryUtils.jsm
> 
> I'm fairly sure not all of these should be necessary in the content
> processes. Folks in #telemetry will know more.

Currently only ClientID.jsm & TelemetrySend.jsm are not needed in the content process. Filed bug 1354041.
TelemetrySession.jsm can be parent-only in the future, but has some unsolved blockers.

Comment 11

[Mark Banner (:standard8)]

Just to note: be careful of shared global scope for frame scripts. There's currently bug 1338907 on reducing that (doing no-undef for ESLint showed this up).

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: notes on startup jsms (updated again)

Comment 13

[Andrew McCreight [:mccr8]]

With erahm's help, I looked at the startup memory from I think just before these various blocking bugs landed, and compared it to what it is now. The numbers are a little noisy, but at least comparing these two individual reports, the memory for a single content process is down by 2.7MB, which is a nice improvement. Most of that is from zones, as you'd expect, but heap overhead, heap unclassified and xpconnect are down a decent amount.

-2.71 MB (100.0%) -- explicit
├──-2.06 MB (76.17%) ++ js-non-window
├──-0.18 MB (06.51%) ++ heap-overhead
├──-0.25 MB (09.13%) ── heap-unclassified
├──-0.13 MB (04.69%) ++ layout
├──-0.09 MB (03.31%) ++ xpconnect
└──-0.01 MB (00.20%) ++ (8 tiny)

resident peak went up which is a little odd.

 -1.66 MB ── heap-allocated
 -3.00 MB ── heap-mapped
  0.02 MB ── js-main-runtime-temporary-peak
   -2,858 ── page-faults-soft
 -3.69 MB ── resident
 15.58 MB ── resident-peak
 -3.26 MB ── resident-unique
-29.61 MB ── vsize

Comment 15

[Andrew McCreight [:mccr8]]

I think this meta bug has served its purpose. We don't load very many JSMs in the content process nowadays.