Closed Bug 1351690 Opened 5 years ago Closed 4 years ago

Lazily load pdf.js in the content process

Categories

(Firefox :: PDF Viewer, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 files, 3 obsolete files)

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.
Thanks Andrew for filing and taking this!
Depends on: 1352218
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.
No longer blocks: SyncIPC
Whiteboard: [MemShrink] → [MemShrink:P2]
Tobias: can we address this issue while you're in the PDF.js sources?
Flags: needinfo?(tschneider)
Yeah, would make sense. Will have a look at it.
Flags: needinfo?(tschneider)
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.
Assignee: continuation → nobody
Priority: -- → P3
Assignee: nobody → continuation
Kris, it sounded like you had some ideas about moving registration to a manifest. Can you elaborate?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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.
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 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
Attachment #8980106 - Flags: review?(bdahl) → review+
Comment on attachment 8980103 [details]
Bug 1351690, part 1b - Don't unload the .jsm on unregister.

https://reviewboard.mozilla.org/r/246262/#review254212
Attachment #8980103 - Flags: review?(bdahl) → review+
Comment on attachment 8980104 [details]
Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

https://reviewboard.mozilla.org/r/246264/#review254214
Attachment #8980104 - Flags: review?(bdahl) → review+
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
Attachment #8980105 - Flags: review?(bdahl) → review+
Looks good, thanks!
Pushed by amccreight@mozilla.com:
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
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.
Flags: needinfo?(continuation)
Attachment #8980104 - Attachment is obsolete: true
Attachment #8980105 - Attachment is obsolete: true
Attachment #8980106 - Attachment is obsolete: true
Attachment #8980103 - Flags: review+ → review?(bdahl)
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 on attachment 8982662 [details]
Bug 1351690, part 1 - Split out pdf.js stream registration into another jsm.

https://reviewboard.mozilla.org/r/248612/#review255206
Attachment #8982662 - Flags: review?(bdahl) → review+
Comment on attachment 8982663 [details]
Bug 1351690, part 2 - Specialize Factory into StreamConverterFactory.

https://reviewboard.mozilla.org/r/248614/#review255210
Attachment #8982663 - Flags: review?(bdahl) → review+
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
Attachment #8982664 - Flags: review?(bdahl) → review+
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
Attachment #8982665 - Flags: review?(bdahl) → review+
Attachment #8980103 - Flags: review?(bdahl) → review+
Thanks for the reviews!
Pushed by amccreight@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.