Closed Bug 1139678 Opened 9 years ago Closed 9 years ago

<Regression> Fx-Team-Non-PGO - Tp5 Optimized caused by background readability parsing

Categories

(Toolkit :: Reader Mode, defect, P1)

defect
Points:
13

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(3 files, 1 obsolete file)

Looks like landing bug 1124011 caused some perf issues.

On IRC, Mossop and Gijs were kicking around the idea of creating some dumber "first pass" parsing algorithm to determine whether or not to show the reader view button, rather than parsing the entire page on every page load only to determine whether or not to show the button. This might be hard, but it could be worth exploring. The downside of this would be that when you click the button, we would have to go to that real parsing, rather than automatically having that article data already stored in memory.

Anything we do here to make performance better on desktop would also improve mobile, where I assume we just decided to eat a perf regression when we implemented reader mode.

Full details:

Regression: Fx-Team-Non-PGO - Tp5 Optimized (Private Bytes) - Ubuntu HW 12.04 x64 - 3.71% increase
--------------------------------------------------------------------------------------------------
    Previous: avg 543199000.000 stddev 2014839.222 of 12 runs up to revision 7230951c2c5b
    New     : avg 563358333.333 stddev 1251018.300 of 12 runs since revision 5cd9065e6cad
    Change  : +20159333.333 (3.71% / z=10.005)
    Graph   : http://mzl.la/1zLnK9C

Regression: Fx-Team-Non-PGO - Tp5 Optimized (Main RSS) - Ubuntu HW 12.04 x64 - 4.01% increase
---------------------------------------------------------------------------------------------
    Previous: avg 212202750.000 stddev 939392.950 of 12 runs up to revision 7230951c2c5b
    New     : avg 220703916.667 stddev 1682620.762 of 12 runs since revision 5cd9065e6cad
    Change  : +8501166.667 (4.01% / z=9.050)
    Graph   : http://mzl.la/1zLnKq0

Regression: Fx-Team-Non-PGO - Tp5 Optimized - Ubuntu HW 12.04 x64 - 3.66% increase
----------------------------------------------------------------------------------
    Previous: avg 279.729 stddev 3.388 of 12 runs up to revision 7230951c2c5b
    New     : avg 289.961 stddev 3.224 of 12 runs since revision 5cd9065e6cad
    Change  : +10.232 (3.66% / z=3.020)
    Graph   : http://mzl.la/1zLnI1p

Regression: Fx-Team-Non-PGO - Tp5 Optimized (Private Bytes) - Ubuntu HW 12.04 - 3.36% increase
----------------------------------------------------------------------------------------------
    Previous: avg 485740833.333 stddev 2679594.808 of 12 runs up to revision 7230951c2c5b
    New     : avg 502041416.667 stddev 1013771.577 of 12 runs since revision 5cd9065e6cad
    Change  : +16300583.333 (3.36% / z=6.083)
    Graph   : http://mzl.la/1zLobRj

Regression: Fx-Team-Non-PGO - Tp5 Optimized (Main RSS) - Ubuntu HW 12.04 - 4.05% increase
-----------------------------------------------------------------------------------------
    Previous: avg 163524083.333 stddev 566201.933 of 12 runs up to revision 7230951c2c5b
    New     : avg 170153333.333 stddev 530955.285 of 12 runs since revision 5cd9065e6cad
    Change  : +6629250.000 (4.05% / z=11.708)
    Graph   : http://mzl.la/1zLoefG

Regression: Fx-Team-Non-PGO - Tp5 Optimized - Ubuntu HW 12.04 - 4.01% increase
------------------------------------------------------------------------------
    Previous: avg 324.458 stddev 3.397 of 12 runs up to revision 7230951c2c5b
    New     : avg 337.483 stddev 4.889 of 12 runs since revision 5cd9065e6cad
    Change  : +13.025 (4.01% / z=3.835)
    Graph   : http://mzl.la/1zLocol
On IRC, Gijs pointed out that these look like memory regressions, not time-spent regressions. jmaher, is that what's going on here?

That would make a lot of sense, since this background parsing does take up a non-trivial amount of memory.
Flags: needinfo?(jmaher)
here is a list of the regressions:
http://alertmanager.allizom.org:8080/alerts.html?rev=5cd9065e6cad&showAll=1&testIndex=0&platIndex=0

"private bytes" and "main rss" are memory.  There is a large %CPU increase on winxp, and tp5 optimized on linux32 and 64 has regressed as well.

thanks for filing a bug and looking at this.  Expect more alerts mid day tomorrow when mozilla-central generates enough data, likewise fx-team, and of course the pgo versions.  What will be interesting is seeing what pgo regressions show up out of this as we ship pgo.

A few options for looking into this:
* try server: try: -b o -p linux,win32 -u none -t tp5o  # add "mozharness: --spsProfile" to generate profile data
* running locally- ping me offline for the tp5 pageset

I guess a question is- do we use readability on desktop?  Luckily this is for winxp and linux (not large consumers of firefox), but the sum of all 3 platforms starts to be worthwhile.
Flags: needinfo?(jmaher)
Blocks: 1140045
(In reply to Joel Maher (:jmaher) from comment #2)

> I guess a question is- do we use readability on desktop?  Luckily this is
> for winxp and linux (not large consumers of firefox), but the sum of all 3
> platforms starts to be worthwhile.

Yes, this regression was caused by turning the feature on for desktop :/

I just disabled this again on Nightly, but we'll need to address this.
(In reply to :Margaret Leibovic from comment #0)

> On IRC, Mossop and Gijs were kicking around the idea of creating some dumber
> "first pass" parsing algorithm to determine whether or not to show the
> reader view button, rather than parsing the entire page on every page load
> only to determine whether or not to show the button. This might be hard, but
> it could be worth exploring. The downside of this would be that when you
> click the button, we would have to go to that real parsing, rather than
> automatically having that article data already stored in memory.

Interestingly enough, we used to do this on Android:
http://hg.mozilla.org/mozilla-central/rev/4ed72e80519c

It looks like we stopped doing it because we moved the parsing logic to a worker, so we thought we could get away with doing all the work upfront, since it was happening in the background.

Maybe we should try adding some of that logic back.
Priority: -- → P1
Flags: qe-verify?
Flags: firefox-backlog+
Blocks: 1132074
Points: --- → 13
Keywords: perf
Whiteboard: [talos_regression]
I re-enabled this feature last night, and I thought this would be better after bug 1140172, but it appears to be even worse :(

The emails are still coming in, but some are even as bad as 39%

Regression: Fx-Team-Non-PGO - Tp5 Optimized (Main RSS) - Ubuntu HW 12.04 - 39.1% increase
-----------------------------------------------------------------------------------------
    Previous: avg 162798583.333 stddev 546667.999 of 12 runs up to revision db75b48611b5
    New     : avg 226407250.000 stddev 2605747.114 of 12 runs since revision c186a66948fb
    Change  : +63608666.667 (39.1% / z=116.357)
    Graph   : http://mzl.la/1AdqGwK
I believe margaret is working on this.
Assignee: nobody → margaret.leibovic
Margaret, if you think there's something manual QA should look at here, please flip the qe-verify flag.
Flags: qe-verify? → qe-verify-
Following some discussions about these performance issues, I'm going to try removing the background parsing logic, and just do the parsing when the user clicks the reader button (this means needing to change the logic we use for determining whether or not to show the button).

To sanity check this approach, I threw together a quick patch to just avoid the background parsing, and this should resolve the talos regression:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f16ed729e0

Few issues with this:

* The current flow with loading reader view is that we open about:reader with a URL parameter, then we check to see if we have a saved parsed article for that browser that matches that URL. The fallback if this doesn't work is to do an XHR to get the page content. This makes sense if the user is directly loading an about:reader URL (e.g. clicking on a reading list item), but if they're clicking on the reader button in the toolbar, we should just use the DOM that currently exists.

* If Readability fails to find an article, we'll just reload the current page. To fix this, we should just make Readability better at returning *something*.

* There are some UX issues to address if we always show the button. Bug 1142265 was filed about this.
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
/r/5469 - Bug 1139678 - Don't do reader parse until user clicks on reader button.

Pull down this commit:

hg pull review -r 1c2f7ea25fe4db60d42b722901c761fbb8a6661a
Attachment #8578180 - Flags: review?(bnicholson)
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

https://reviewboard.mozilla.org/r/5467/#review4409

::: toolkit/components/reader/ReaderMode.jsm
(Diff revision 1)
> +  isProbablyReaderable: function(doc) {

The function name/comments make it sound like we're doing some preliminary check to see if a page is reader-able, but's really only a simple URI filter. Maybe rename this?
Attachment #8578180 - Flags: review?(bnicholson)
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

https://reviewboard.mozilla.org/r/5467/#review4411

Ship It!
Attachment #8578180 - Flags: review+
Blocks: 1143844
Doh, I forgot to update the test that checks to make sure the button isn't shown on a page we can't parse as an article. Here's a try push with the updated test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=456e3bc1ec49
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

/r/5469 - Bug 1139678 - Don't do reader parse until user clicks on reader button. r=bnicholson
/r/5563 - Bug 1139678 - (Part 2) Update non-reader-able test URL in browser_readerMode.js. r=Gijs
/r/5565 - Bug 1139678 - (Part 3) Don't try interacting with the window in AboutReader if it has been unloaded. r=Gijs

Pull down these commits:

hg pull review -r f49a9f306bf4544573eb18c0bcd38b23a22b3ce6
Attachment #8578180 - Flags: review?(gijskruitbosch+bugs)
Attachment #8578180 - Flags: review?(bnicholson)
Attachment #8578180 - Flags: review+
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

https://reviewboard.mozilla.org/r/5467/#review4559

Ship It!
Attachment #8578180 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8578180 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/e02c32b3aabd
https://hg.mozilla.org/mozilla-central/rev/fd89d5769d1c
https://hg.mozilla.org/mozilla-central/rev/cf65e2ae03f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [talos_regression][fixed-in-fx-team] → [talos_regression]
Target Milestone: --- → mozilla39
There are some regression on mozilla-aurora raised: http://alertmanager.allizom.org:8080/alerts.html?rev=cb0db44ce60e&showAll=1&testIndex=0&platIndex=0

It shows that it is because of reader mode: http://hg.mozilla.org/releases/mozilla-aurora/rev/cb0db44ce60e , is it expected to have these?
Margaret, are the regressions shown in comment 27 as expected?
Flags: needinfo?(margaret.leibovic)
(In reply to Vaibhav (:vaibhav1994) from comment #28)
> Margaret, are the regressions shown in comment 27 as expected?

No, these weren't expected, but I'm not sure what we can do to fix these at the moment. I thought we had fixed the original regressions, but I didn't look closely enough to ensure that we reached baseline levels again.

I would suspect bug 1143844 could be at fault, although we did check to make sure that didn't cause tp5 regressions before landing.

There is also bug 1147487 on file, and it's odd that that regression didn't appear also after uplifting this feature.
Flags: needinfo?(margaret.leibovic)
Vaibhav, I think this may be the same thing as bug 1147554. Reader mode had already been enabled on fx-team and had regressions fixed there. I think Reading List may have some things that it may need to take care of.
Flags: needinfo?(vaibhavmagarwal)
Ok, I have commented in bug 1147554
Flags: needinfo?(vaibhavmagarwal)
Blocks: 1142183
Blocks: 1150656
Attachment #8578180 - Attachment is obsolete: true
Attachment #8619664 - Flags: review+
Attachment #8619665 - Flags: review+
Attachment #8619666 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: