792603 - Disable automatic reader mode parsing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Brian Nicholson (:bnicholson)]

Visiting any page starts a web worker that does a readability parse, and this can consume tens of MB of memory. For ARMv6, we should consider always showing the reader icon and lazily doing the parse when the icon is clicked.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Have a WIP that I formulated based on code inspection that I pushed to try to get a build (since my machine is otherwise occupied at the moment). I will test the try build on a low-mem device once it's done.

https://tbpl.mozilla.org/?tree=Try&rev=27f8f51182f5

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/3) Add delayed reader mode parsing for low-memory devices

So my WIP was really simplistic and unfortunately didn't work too well. I have a new set of patches that behaves better.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/3) Display the progress spinner while we do the delayed parse

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/3) Make the stop button on the delayed parse spinner cancel the reader mode switch

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Also CC'ing lucasr; the more feedback the better since these changes were a little tricky to get right.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: about:memory data with reader mode parsing on and off

mfinkle, bnicholson, ibarlow, and I had a discussion on IRC about this. The upshot of it is basically that we need to gather more data about how much this would help and how often the user would end up having a failed parse to deal with.

I grabbed about:memory dumps from two builds, one from inbound cset cd8533b0fae7 and one from try cset b1bfa13fe381 which is the inbound one + a patch to disable reader mode parsing completely. The full datasets are attached. I loaded a few pages of different sizes to see how much memory impact there was. The numbers listed below show the resident memory on the inbound and try builds after page load, as well as the memory used by ReaderWorker.js as reported by about:memory. (Note that the difference in resident memory might be different than just what is used by ReaderWorker.js, partly because of other reader mode data lying around, and partly because I loaded the page off the network and there might have been different ads etc. on the two loads).

Still, there are significant savings when the reader mode parse is turned off, specially for the larger pages.

Page                                | Inbound  | Try      | ReaderWorker.js
---------------------------------------------------------------------------
personal blog page (300 elements)   | 66117632 | 63369216 | 2.79 MB
nytimes.com article (1100 elements) | 98963456 | 85897216 | 4.12 MB
HN comments page (2500 elements)    | 86519808 | 75841536 | 9.17 MB

Given this data, one possibility is to reduce the 3000 element threshold beyond which we don't do the reader mode parse. However that will also reduce the usefulness of reader mode because more pages won't have the reader icon. It might be better to just turn it off altogether.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 682676 [details] [diff] [review]
(1/3) Add delayed reader mode parsing for low-memory devices

Review of attachment 682676 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself looks good but I'm not entirely convinced it's worth offering Reader Mode on lower-end devices in those terms. Even if we don't always run the readability check on each page, the parsing will still raise memory footprint quite a bit once reader mode is activated. As I said, need to check with UX if they're OK with the always-on reader button and the potentially long delay to activate the reader.

Giving r+ but you should get approval from UX.

::: mobile/android/base/Tab.java
@@ +418,5 @@
>  
>      public void readerMode() {
> +        switch (mReaderState) {
> +            case UNDECIDED:
> +                // TODO: display UI indication that we are processing since this may take a while

This kinda defeats one of the core aspects of this feature: that you can quickly switch to reader mode. The feature might become a bit too frustrating to use if it's slow to switch.

::: mobile/android/chrome/content/browser.js
@@ +7261,5 @@
>  
> +        if (Cc["@mozilla.org/xpcom/memory-service;1"].getService(Ci.nsIMemory).isLowMemoryPlatform()) {
> +          // on low-memory platforms, don't do the relatively expensive parse of the page to see if
> +          // it is an article. instead, just assume that it is and display the reader mode icon, and
> +          // and then when the user clicks on the icon do the actual check.

Need to check with UX guys if that's acceptable. This will make the feature generally unpredictable. Not sure it's a win.

@@ +7329,5 @@
> +        type: "Content:ReaderState",
> +        tabID: tabId,
> +        state: 0 /* Tab.ReaderState.DISABLED */
> +      }
> +    });

Maybe simply rename this to something like sendReaderState(tabId, state) and use it everywhere?

Comment 8

[Lucas Rocha (:lucasr)]

Ian, Madhava, what is your take on this?

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 682677 [details] [diff] [review]
(2/3) Display the progress spinner while we do the delayed parse

Review of attachment 682677 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserToolbar.java
@@ +398,5 @@
>                      updateForwardButton(tab.canDoForward());
>                  }
>                  break;
> +            case READER_PARSING:
> +                setProgressVisibility((Boolean)data);

If you do that, you'll probably have to make the stop button do "something" when clicked. Otherwise if will just feel broken while the parsing is running. Probably worth checking with UX what to do here.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 682676 [details] [diff] [review]
(1/3) Add delayed reader mode parsing for low-memory devices

Review of attachment 682676 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I missed one important point here. The about:reader is already able to handle the case where the pre-parsed article is not available in the tab — in which case it will do all the parsing from scratch before showing the content. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#356
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7233

You probably want to parse document from tab instead of URL when the pre-parsed article is not available though. So, actually, you don't really need the delayed parsing bits from this patch. Just invoke reader mode and it will try to do the right thing for you.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 682678 [details] [diff] [review]
(3/3) Make the stop button on the delayed parse spinner cancel the reader mode switch

Review of attachment 682678 [details] [diff] [review]:
-----------------------------------------------------------------

If you just start reader mode instead of the delayed parsing bits you won't really need this. The reader UI already has a progress UI (a minimal one but still...)

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 682677 [details] [diff] [review]
(2/3) Display the progress spinner while we do the delayed parse

Review of attachment 682677 [details] [diff] [review]:
-----------------------------------------------------------------

You're doing the right thing with the stop button in patch 3/3 but I still think it's simpler to just show the progress state in about:reader.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Disable reader mode on low-mem devices

I think based on the feedback so far and the discussion on IRC yesterday people are leaning towards just turning it off entirely rather than doing the delayed parse. I don't feel strongly about it either way but this saves more memory and is a far simpler patch so it's fine by me.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 683599 [details] [diff] [review]
Disable reader mode on low-mem devices

Review of attachment 683599 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good (if we decide to disable it on low-end devices)

Comment 15

[Kartikaya Gupta (email:[e-mail])]

ibarlow agreed on IRC that disabling it is currently the best option, so landing this patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e055ce38b6f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e055ce38b6f