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

NEW
Assigned to

Status

()

enhancement
2 years ago
5 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Depends on 7 bugs, Blocks 1 bug, {meta})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [overhead:noted])

Attachments

(3 attachments, 2 obsolete attachments)

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.
Depends on: 1350469, 1349389
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.
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.
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
(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.
Depends on: 1350878
Depends on: 1351008
Posted file notes on startup jsms (obsolete) —
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.
Depends on: 1351385
Depends on: 1351690
Depends on: 1352522
Depends on: 1352541
Depends on: 1317697
Depends on: 1353081
Depends on: 1353089
Depends on: 1250487
Depends on: 1353174
Posted file notes on startup jsms (updated) (obsolete) —
Attachment #8851813 - Attachment is obsolete: true
See Also: → 1352982
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...
Blocks: 1353816
Depends on: 837230
There are other ideas of components that could be removed/merged blocking bug 986323 (e.g. bug 986501, bug 986503, bug 988096).
Depends on: 1354036
Depends on: 1354039
(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.
Depends on: 1354328
Depends on: 1354341
Depends on: 1354820
Depends on: 1355202
Depends on: 1355248
Depends on: 1355533
Depends on: 1354041
Depends on: 1355543
Depends on: 1355547
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).
Attachment #8854213 - Attachment is obsolete: true
Depends on: 1355611
Depends on: 1355619
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
Depends on: 1356572
Depends on: 1356594
Depends on: 1357731
Duplicate of this bug: 1317696
Depends on: 1385954
No longer depends on: 1357517
See Also: → 1425524
Depends on: 1472491
Whiteboard: [overhead:noted]
Summary: Reduce the number of jsms loaded in content processes → [meta] Reduce the number of jsms loaded in content processes
You need to log in before you can comment on or make changes to this bug.