1351690 - Lazily load pdf.js in the content process

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P3)

Description

[Andrew McCreight [:mccr8]]

We load 4 .jsms in the content process at startup for pdf.js, from pdfjschildbootstrap.js. With 4 content processes, that's about 480kb of memory.

There are a few reasons for this:

1. PdfStreamConverter.jsm is loaded so that it can be used as a factory. However, we can avoid loading it until somebody actually creates a factory by moving the classID etc. stuff out of the converter file.

2. PdfContentUtils.init() listens for a message manager message "PDFJS:Child:refreshSettings", and also for the observer notice "quit-application" (the latter just to unregister itself from the mm). This can be avoided by creating a new class to deal with the receiveMessage stuff in the bootstrap file.

3. PdfJs.updateRegistration() checks if pdf.js is enabled. As part of that, it calls PdfjsContentUtils.isDefaultHandlerApp(), which sends a sync message to the parent (which is also not good). I'm guessing that the value of enabled() is the same in the parent and the child, and the bootstrap file is loaded by a loadProcessScript() call, which I assume is in the parent. It should be possible, then, to have two specialized bootstrap files (one for the enabled case, one for the !enabled case) and have the parent check the value and call the appropriate script.

4. In the enabled case, PdfJs.updateRegistration() registers a factory. I think we should be able to move that into the bootstrap file. It does have some state about whether it is registered, but maybe we use the registrar to check if it is registered, as is done in the JSON viewer. In the !enabled case, I think it removes the factory, but I'm guessing that we will never have the factory registered in this case, so we don't need to do anything.

I'll be sure to submit upstream pull requests. I just wanted to have these on file in bugzilla.

Comment 1

[(no longer active)]

Thanks Andrew for filing and taking this!

Comment 2

[Andrew McCreight [:mccr8]]

I split (3) into bug 1352218.

The rest may be a little messier to fix because the Firefox addon code has to do the same stuff as the bootstrap script and I'd like to avoid too much duplication of code. Maybe I can figure something out.

Comment 3

[Jet Villegas (inactive)]

Tobias: can we address this issue while you're in the PDF.js sources?

Comment 4

[Tobias Schneider [:tobytailor]]

Yeah, would make sense. Will have a look at it.

Comment 5

[Andrew McCreight [:mccr8]]

The basic idea here is to move the code that registers observers etc. into the bootstrap script. It may be a little annoying to do because the Firefox addon version of the code also wants to register the same stuff, but I think doesn't use the bootstrap script. I also have some patches in flight to change the bootstrap code a little bit.

Comment 6

[Eric Rahm [:erahm]]

Kris, it sounded like you had some ideas about moving registration to a manifest. Can you elaborate?

Comment 7

[Kris Maglione [:kmag]]

So, basically, the way we normally load JS services is to declare their contracts and categories in a manifest, which causes them to be loaded lazily when they're first instantiated. This is how we register the HTTP index converter, for instance:

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/toolkit/components/utils/utils.manifest#6-8

The way PDF.js registers its converters is... weird. But I'm pretty sure we can get away with just registering its converter contracts in the manifest, and letting everything else happen lazily:

  @mozilla.org/streamconv;1?from=application/pdf&to=*/*
  @mozilla.org/streamconv;1?from=application/pdf&to=text/html

It does some stuff with the mime service to make sure it's the default handler, too, but that only needs to happen in the content process. And it unregisters any existing Gecko-Content-Viewers category entry for the PDF content type, but we don't support non-Flash plugins anymore, so that shouldn't be necessary.

Comment 8

[Andrew McCreight [:mccr8]]

Well, I'll keep that in mind for the future, but I already have some patches that are simple and that work, so I'm just going to go ahead and try to land those rather than digging further into how PDF.js loads.

Comment 9

[Andrew McCreight [:mccr8]]

I don't know how reliable these numbers are, but looking at two memory reports before and two after it looks like this might reduce content process memory usage by a few hundred kb, split between scripts and classes. That sounds kind of high to me, but who knows. We'll see if anything shows up on the new about:blank AWSY test.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 1b - Don't unload the .jsm on unregister.

The goal of these patches is to load neither PdfJs.jsm nor
PdfStreamConverter.jsm in a content process unless the user is viewing
a PDF, to save memory.

This first patch creates a small stub JSM to do the stream
registration, and calls it directly in the content process, avoiding
one way we load PdfJs.jsm. The existing registration methods are kept
for the main process.

Review commit: https://reviewboard.mozilla.org/r/246262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246262/

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

Factory is only ever passed PdfStreamConverter, so specialize the
registration method and rename the class.  Also, classID2 is always
non-null for PdfStreamConverter, so drop the check.

Review commit: https://reviewboard.mozilla.org/r/246264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246264/

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 3 - Move stream converter XPCOM registration constants into the registration file.

As with the last patch, the factory is only used for a single class,
so move the constants closer to where they are used. This will allow
us to register the stream converter without loading the stream
converter JSM.

Review commit: https://reviewboard.mozilla.org/r/246266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246266/

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 4 - Only load the stream converter when we try to view a pdf.

This inlines and simplifies the call to XPCOMUtils._getFactory,
because otherwise passing PdfStreamConverter appears to resolve it
immediately, loading the JSM. (The stream converter prototype does not
have a property _xpcom_factory, so there's no need for the check.)

Once that is done, we can just lazily load the stream converter JSM to
keep it from being loaded on startup.

This patch also checks that the stream converter is not loaded at
startup in the main process or the content process, and that PdfJs.jsm
is not loaded at startup in the content process. It needs to be loaded
in the main process to watch for some prefs.

Review commit: https://reviewboard.mozilla.org/r/246268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246268/

Comment 14

[Brendan Dahl [:bdahl]]

Comment on attachment 8980106 [details]
Bug 1351690, part 4 - Only load the stream converter when we try to view a pdf.

https://reviewboard.mozilla.org/r/246268/#review254218

Comment 15

[Brendan Dahl [:bdahl]]

Comment on attachment 8980103 [details]
Bug 1351690, part 1b - Don't unload the .jsm on unregister.

https://reviewboard.mozilla.org/r/246262/#review254212

Comment 16

[Brendan Dahl [:bdahl]]

Comment on attachment 8980104 [details]
Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

https://reviewboard.mozilla.org/r/246264/#review254214

Comment 17

[Brendan Dahl [:bdahl]]

Comment on attachment 8980105 [details]
Bug 1351690, part 3 - Move stream converter XPCOM registration constants into the registration file.

https://reviewboard.mozilla.org/r/246266/#review254216

Comment 18

[Brendan Dahl [:bdahl]]

Looks good, thanks!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4219d3de5e8b
part 1 - Split out pdf.js stream registration into another jsm. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/64d513f3c02b
part 2 - Specialize Factory into StreamConverterFactory. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/64082649b185
part 3 - Move stream converter XPCOM registration constants into the registration file. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/8def346f7119
part 4 - Only load the stream converter when we try to view a pdf. r=bdahl

Comment 20

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome at browser/extensions/pdfjs/test/browser_pdfjs_views.js and browser/extensions/pdfjs/test/browser_pdfjs_zoom.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8def346f71195bad8e90b55584dea61266e48247

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=181074206&repo=autoland&lineNumber=4120
and 
https://treeherder.mozilla.org/logviewer.html#?job_id=181076488&repo=autoland&lineNumber=4108


Backout: https://hg.mozilla.org/integration/autoland/rev/d5eed637f5dd1f04bf0b4366c2ad91a3482a0686

Comment 21

[Andrew McCreight [:mccr8]]

That's strange. It is only failing on Linux32 and OSX. Of course, most platforms seem to have not run the tests. I also can't reproduce locally on Linux 64, either on opt or debug. I'll try to repro on OSX.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8980103 [details]
Bug 1351690, part 1b - Don't unload the .jsm on unregister.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246262/diff/1-2/

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 1 - Split out pdf.js stream registration into another jsm.

The goal of these patches is to load neither PdfJs.jsm nor
PdfStreamConverter.jsm in a content process unless the user is viewing
a PDF, to save memory.

This first patch creates a small stub JSM to do the stream
registration, and calls it directly in the content process, avoiding
one way we load PdfJs.jsm. The existing registration methods are kept
for the main process.

Review commit: https://reviewboard.mozilla.org/r/248612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248612/

Comment 24

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

Factory is only ever passed PdfStreamConverter, so specialize the
registration method and rename the class.  Also, classID2 is always
non-null for PdfStreamConverter, so drop the check.

Review commit: https://reviewboard.mozilla.org/r/248614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248614/

Comment 25

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 3 - Move stream converter XPCOM registration constants into the registration file.

As with the last patch, the factory is only used for a single class,
so move the constants closer to where they are used. This will allow
us to register the stream converter without loading the stream
converter JSM.

Review commit: https://reviewboard.mozilla.org/r/248616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248616/

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351690, part 4 - Only load the stream converter when we try to view a pdf.

This inlines and simplifies the call to XPCOMUtils._getFactory,
because otherwise passing PdfStreamConverter appears to resolve it
immediately, loading the JSM. (The stream converter prototype does not
have a property _xpcom_factory, so there's no need for the check.)

Once that is done, we can just lazily load the stream converter JSM to
keep it from being loaded on startup.

This patch also checks that the stream converter is not loaded at
startup in the main process or the content process, and that PdfJs.jsm
is not loaded at startup in the content process. It needs to be loaded
in the main process to watch for some prefs.

Review commit: https://reviewboard.mozilla.org/r/248618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248618/

Comment 27

[Andrew McCreight [:mccr8]]

Sorry, I messed up when attaching these files. Only 1b needs an actual review. The rest of the patches are identical, so please just go through and re-r+ them again...

Comment 28

[Brendan Dahl [:bdahl]]

Comment on attachment 8982662 [details]
Bug 1351690, part 1 - Split out pdf.js stream registration into another jsm.

https://reviewboard.mozilla.org/r/248612/#review255206

Comment 29

[Brendan Dahl [:bdahl]]

Comment on attachment 8982663 [details]
Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

https://reviewboard.mozilla.org/r/248614/#review255210

Comment 30

[Brendan Dahl [:bdahl]]

Comment on attachment 8982664 [details]
Bug 1351690, part 3 - Move stream converter XPCOM registration constants into the registration file.

https://reviewboard.mozilla.org/r/248616/#review255212

Comment 31

[Brendan Dahl [:bdahl]]

Comment on attachment 8982665 [details]
Bug 1351690, part 4 - Only load the stream converter when we try to view a pdf.

https://reviewboard.mozilla.org/r/248618/#review255214

Comment 32

[Andrew McCreight [:mccr8]]

Thanks for the reviews!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1d893c1e41f
part 1 - Split out pdf.js stream registration into another jsm. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/047a3f707837
part 1b - Don't unload the .jsm on unregister. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/21a2a25d8fa0
part 2 - Specialize Factory into StreamConverterFactory. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/5eb08c5b8492
part 3 - Move stream converter XPCOM registration constants into the registration file. r=bdahl
https://hg.mozilla.org/integration/autoland/rev/a85debb4f781
part 4 - Only load the stream converter when we try to view a pdf. r=bdahl

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c1d893c1e41f
https://hg.mozilla.org/mozilla-central/rev/047a3f707837
https://hg.mozilla.org/mozilla-central/rev/21a2a25d8fa0
https://hg.mozilla.org/mozilla-central/rev/5eb08c5b8492
https://hg.mozilla.org/mozilla-central/rev/a85debb4f781