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)
Toolkit
Reader Mode
Tracking
()
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
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
(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.
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Updated•9 years ago
|
Points: --- → 13
Assignee | ||
Comment 5•9 years ago
|
||
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 7•9 years ago
|
||
Margaret, if you think there's something manual QA should look at here, please flip the qe-verify flag.
Flags: qe-verify? → qe-verify-
Assignee | ||
Comment 8•9 years ago
|
||
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.
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Comment 9•9 years ago
|
||
forgot to update this with the link to the alerts when it landed again: http://alertmanager.allizom.org:8080/alerts.html?rev=c186a66948fb&showAll=1&testIndex=0&platIndex=0 and for pgo: http://alertmanager.allizom.org:8080/alerts.html?rev=bc2b7db48fba&showAll=1&testIndex=0&platIndex=0
Assignee | ||
Comment 10•9 years ago
|
||
/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 11•9 years ago
|
||
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 12•9 years ago
|
||
Comment on attachment 8578180 [details] MozReview Request: bz://1139678/margaret https://reviewboard.mozilla.org/r/5467/#review4411 Ship It!
Attachment #8578180 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/35a52bd476cb
Backed out in https://hg.mozilla.org/integration/fx-team/rev/a94d9479d749 for e10s bc1 orange: https://treeherder.mozilla.org/logviewer.html#?job_id=2291485&repo=fx-team
Flags: needinfo?(margaret.leibovic)
Looks like non-e10s fails too: https://treeherder.mozilla.org/logviewer.html#?job_id=2290663&repo=fx-team
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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 20•9 years ago
|
||
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+
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82252be0046
Comment 22•9 years ago
|
||
https://reviewboard.mozilla.org/r/5469/#review4567 Ship It!
Updated•9 years ago
|
Attachment #8578180 -
Flags: review?(bnicholson) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8578180 [details] MozReview Request: bz://1139678/margaret https://reviewboard.mozilla.org/r/5467/#review4569
Comment 24•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/e02c32b3aabd remote: https://hg.mozilla.org/integration/fx-team/rev/fd89d5769d1c remote: https://hg.mozilla.org/integration/fx-team/rev/cf65e2ae03f9
Whiteboard: [talos_regression] → [talos_regression][fixed-in-fx-team]
Comment 25•9 years ago
|
||
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
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [talos_regression][fixed-in-fx-team] → [talos_regression]
Target Milestone: --- → mozilla39
Comment 26•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/afede426f944 https://hg.mozilla.org/releases/mozilla-aurora/rev/ead131a382fe https://hg.mozilla.org/releases/mozilla-aurora/rev/b7c0318cd5b0
status-firefox38:
--- → fixed
Comment 27•9 years ago
|
||
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•9 years ago
|
||
Margaret, are the regressions shown in comment 27 as expected?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 29•9 years ago
|
||
(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)
Comment 30•9 years ago
|
||
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)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8578180 -
Attachment is obsolete: true
Attachment #8619664 -
Flags: review+
Attachment #8619665 -
Flags: review+
Attachment #8619666 -
Flags: review+
Assignee | ||
Comment 33•9 years ago
|
||
Assignee | ||
Comment 34•9 years ago
|
||
Assignee | ||
Comment 35•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•