1484413 - Minimize the amount of reader mode code loaded for readerable checking

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, enhancement, P1)

Description

[Kris Maglione [:kmag]]

ReaderMode.jsm includes a huge amount of code which is only needed when actually rendering a page in reader mode, but is loaded into every process in which we check whether a page is readerable too.

The actual code that we need for checking is pretty minimial. I'm setting the approximate overhead at 30K, because that's what we currently wind up with about:blank. But for any process that ever loads an http or https URL, it's really *way* higher, since then we wind up loading Readability.js, too... :/

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r=Gijs

Most of the ReaderMode.jsm and Readability.js code is only needed when we
actually need to render a document in reader mode, but also winds up loaded
into any process where we ever check if a page is readerable. This winds up
wasting a huge amount of memory (and probably a huge amount of CPU time)
loading code which is almost never used.

This patch splits ReaderMode.jsm into two modules, one for checking
readability, one for actually entering reader mode. It also separates out the
isProbablyReaderable checks from Readability.js, since the overhead of loading
that script before it's needed is unsupportable.

This means we're probably going to need some effort to keep Readerable.jsm and
Readability.js in sync, but the code in question is pretty trivial, so it
shouldn't be too difficult.

Comment 2

[:Gijs (he/him)]

I'm not going to be able to get to this review today, having just returned from PTO and because phabricator didn't block out my review queue.

On the whole, though obviously for performance reasons we need to do *something* here, I'm pretty uncomfortable with this change and the duplication it creates.

Can we instead move the `isProbablyReaderable` code to its own file that's loaded by Readability.js , so we avoid duplicating the relevant bits of code ? Is that not possible for some reason?

Comment 3

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #2)
> I'm not going to be able to get to this review today, having just returned
> from PTO and because phabricator didn't block out my review queue.

Yeah, sorry about that. I just found out about that issue this weekend. I guess I'm going to need to start checking Bugzilla before I submit patches...

> Can we instead move the `isProbablyReaderable` code to its own file that's
> loaded by Readability.js , so we avoid duplicating the relevant bits of code
> ? Is that not possible for some reason?

We probably could, but Readability.js is an upstream library, and that makes things complicated. Aside from the complexity that it tends to add for supporting browsers without support for modern standards, there's also the complexity of it needing to be able to load scripts in a bunch of different sorts of environments. It would require changing all consumers to load two separate scripts in the right order.

And then we'd have to load that script using the subscript loader, which I'd generally rather avoid (especially since it would generally happen late enough that we wouldn't be able to use the pre-loader cache in content processes).

All things considered, the duplicated code just seemed trivial enough that I didn't think it warranted the extra complexity.

Comment 4

[Kris Maglione [:kmag]]

Updated patch contains changes that need to land in the Readability repo, too. I'm submitting them here first, since I think they'll be easier to review that way, and will send a PR there afterwards.

Unfortunately, the patch also wound up including a bunch of unrelated upstream Readability.js changes, which I didn't realize until too late...

Comment 5

[:Gijs (he/him)]

Comment on attachment 9002177 [details]
Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r=Gijs

:Gijs (he/him) has approved the revision.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9735b9bf7b466660a2422bd7689968f114847fc3
Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r=Gijs

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9735b9bf7b46

Comment 8

[:Gijs (he/him)]

I dealt with the github side of this in https://github.com/mozilla/readability/pull/507 . I've filed bug 1516877 to sync up the two versions again.