Minimize the amount of reader mode code loaded for readerable checking

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

(Whiteboard: [overhead:30K])

Attachments

(1 attachment)

Assignee

Description

10 months ago
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... :/
Assignee

Comment 1

10 months ago
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.

Updated

10 months ago
Priority: -- → P1

Comment 2

10 months ago
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?
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 3

10 months ago
(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.
Flags: needinfo?(kmaglione+bmo)

Updated

10 months ago
Attachment #9002177 - Attachment description: Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r?Gijs → Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r=Gijs
Assignee

Comment 4

10 months ago
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

10 months ago
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.
Attachment #9002177 - Flags: review+

Updated

10 months ago
Depends on: 1487044
Assignee

Comment 6

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

Comment 7

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9735b9bf7b46
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 8

6 months ago
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.
You need to log in before you can comment on or make changes to this bug.