Last Comment Bug 1139678 - <Regression> Fx-Team-Non-PGO - Tp5 Optimized caused by background readability parsing
: <Regression> Fx-Team-Non-PGO - Tp5 Optimized caused by background readability...
Status: RESOLVED FIXED
[talos_regression]
: perf, regression
Product: Toolkit
Classification: Components
Component: Reader Mode (show other bugs)
: Trunk
: All All
P1 normal (vote)
: mozilla39
Assigned To: :Margaret Leibovic
:
: :Gijs
Mentors:
Depends on:
Blocks: 1132074 desktop-reader 1124011 1138995 1140045 1142183 1143844 1150656
  Show dependency treegraph
 
Reported: 2015-03-04 16:05 PST by :Margaret Leibovic
Modified: 2015-06-10 08:36 PDT (History)
16 users (show)
mmucci: firefox‑backlog+
andrei.vaida: qe‑verify-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 39.2 - 23 Mar
Points: 13
Has Regression Range: ---
Has STR: ---
fixed
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
MozReview Request: bz://1139678/margaret (39 bytes, text/x-review-board-request)
2015-03-16 12:38 PDT, :Margaret Leibovic
bnicholson: review+
gijskruitbosch+bugs: review+
Details | Review
MozReview Request: Bug 1139678 - (Part 2) Update non-reader-able test URL in browser_readerMode.js. r=Gijs (39 bytes, text/x-review-board-request)
2015-06-10 08:36 PDT, :Margaret Leibovic
gijskruitbosch+bugs: review+
bnicholson: review+
Details | Review
MozReview Request: Bug 1139678 - Don't do reader parse until user clicks on reader button. r=bnicholson (39 bytes, text/x-review-board-request)
2015-06-10 08:36 PDT, :Margaret Leibovic
bnicholson: review+
gijskruitbosch+bugs: review+
Details | Review
MozReview Request: Bug 1139678 - (Part 3) Don't try interacting with the window in AboutReader if it has been unloaded. r=Gijs (39 bytes, text/x-review-board-request)
2015-06-10 08:36 PDT, :Margaret Leibovic
gijskruitbosch+bugs: review+
bnicholson: review+
Details | Review

Description User image :Margaret Leibovic 2015-03-04 16:05:13 PST
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
Comment 1 User image :Margaret Leibovic 2015-03-04 16:10:22 PST
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.
Comment 2 User image Joel Maher ( :jmaher) 2015-03-04 16:52:27 PST
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.
Comment 3 User image :Margaret Leibovic 2015-03-05 12:49:20 PST
(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.
Comment 4 User image :Margaret Leibovic 2015-03-05 14:28:54 PST
(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.
Comment 5 User image :Margaret Leibovic 2015-03-10 09:25:48 PDT
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
Comment 6 User image Justin Dolske [:Dolske] 2015-03-10 21:44:31 PDT
I believe margaret is working on this.
Comment 7 User image Andrei Vaida, QA [:avaida] – please ni? me 2015-03-12 00:54:38 PDT
Margaret, if you think there's something manual QA should look at here, please flip the qe-verify flag.
Comment 8 User image :Margaret Leibovic 2015-03-12 20:17:28 PDT
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.
Comment 10 User image :Margaret Leibovic 2015-03-16 12:38:03 PDT
Created attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

/r/5469 - Bug 1139678 - Don't do reader parse until user clicks on reader button.

Pull down this commit:

hg pull review -r 1c2f7ea25fe4db60d42b722901c761fbb8a6661a
Comment 11 User image Brian Nicholson (:bnicholson) 2015-03-16 13:19:57 PDT
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?
Comment 12 User image Brian Nicholson (:bnicholson) 2015-03-16 13:20:03 PDT
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

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

Ship It!
Comment 15 User image Wes Kocher (:KWierso) 2015-03-16 15:27:48 PDT
Looks like non-e10s fails too: https://treeherder.mozilla.org/logviewer.html#?job_id=2290663&repo=fx-team
Comment 16 User image :Margaret Leibovic 2015-03-17 09:26:37 PDT
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
Comment 17 User image :Margaret Leibovic 2015-03-17 14:51:10 PDT
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
Comment 20 User image :Gijs 2015-03-17 14:54:25 PDT
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

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

Ship It!
Comment 22 User image Brian Nicholson (:bnicholson) 2015-03-17 16:16:26 PDT
https://reviewboard.mozilla.org/r/5469/#review4567

Ship It!
Comment 23 User image Brian Nicholson (:bnicholson) 2015-03-17 16:17:11 PDT
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret

https://reviewboard.mozilla.org/r/5467/#review4569
Comment 27 User image Vaibhav (:vaibhav1994) 2015-03-30 09:16:54 PDT
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?
Comment 28 User image Vaibhav (:vaibhav1994) 2015-03-30 09:19:06 PDT
Margaret, are the regressions shown in comment 27 as expected?
Comment 29 User image :Margaret Leibovic 2015-03-30 12:23:06 PDT
(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.
Comment 30 User image Jared Wein [:jaws] (please needinfo? me) 2015-03-30 12:30:23 PDT
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.
Comment 31 User image Vaibhav (:vaibhav1994) 2015-03-31 07:34:34 PDT
Ok, I have commented in bug 1147554
Comment 32 User image :Margaret Leibovic 2015-06-10 08:36:08 PDT
Comment on attachment 8578180 [details]
MozReview Request: bz://1139678/margaret
Comment 33 User image :Margaret Leibovic 2015-06-10 08:36:08 PDT
Created attachment 8619664 [details]
MozReview Request: Bug 1139678 - (Part 2) Update non-reader-able test URL in browser_readerMode.js. r=Gijs
Comment 34 User image :Margaret Leibovic 2015-06-10 08:36:08 PDT
Created attachment 8619665 [details]
MozReview Request: Bug 1139678 - Don't do reader parse until user clicks on reader button. r=bnicholson
Comment 35 User image :Margaret Leibovic 2015-06-10 08:36:08 PDT
Created attachment 8619666 [details]
MozReview Request: Bug 1139678 - (Part 3) Don't try interacting with the window in AboutReader if it has been unloaded. r=Gijs

Note You need to log in before you can comment on or make changes to this bug.