46 bytes, text/x-phabricator-request
|Details | Review|
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... :/
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.
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?
(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.
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
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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9735b9bf7b466660a2422bd7689968f114847fc3 Bug 1484413: Split ReaderMode.jsm into separate checking and loading components. r=Gijs
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.