Closed Bug 1147487 Opened 9 years ago Closed 9 years ago

21% Linux tsvgr_opacity regression on fx-team PGO (v.39) on March 21, 2015 from push d6f1397b3563

Categories

(Toolkit :: Reader Mode, defect)

Product:
Toolkit
Component:
Reader Mode
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Flags:
Tracking Status
firefox38 --- fixed
firefox39 + fixed
firefox38.0.5 --- fixed
firefox40 --- fixed

People

(Reporter: jmaher, Assigned: Gijs)

References

Details

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

Attachments

(2 files, 2 obsolete files)

don't bother sending reader mode updates when isArticle is false,
(deleted), patch
Margaret
: review+
lizzard
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
MozReview Request: Bug 1147487 - don't try to reader-ize non-HTML documents, r?margaret
(deleted), text/x-review-board-request
Margaret
: review+
Details 
Talos has detected a Firefox performance regression from your patches in commit d6f1397b3563.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=03453271ff96&showAll=1

On the page above you can see Talos alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg-opacity

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux -u none -t svgr  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a tsvgr_opacity

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Monday, or the offending patch will be backed out! ***

Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

What is interesting here is we have a huge win for non-pgo, but a big regression for PGO (only on linux32 is the regression)
Margaret, any thoughts on this?  I think this might just be a random blip in the way pgo works.
Flags: needinfo?(margaret.leibovic)
(In reply to Joel Maher (:jmaher) from comment #0)
> Talos has detected a Firefox performance regression from your patches in
> commit d6f1397b3563.  We need you to address this regression.
> 
> This is a list of all known regressions and improvements related to your bug:
> http://alertmanager.allizom.org:8080/alerts.html?rev=03453271ff96&showAll=1


This webpage makes no sense to me. What are the percentages, and what does the +/- mean? Which is good and which is bad? Why are there no labels for the columns? How come this is listing inbound when the patches landed on fx-team? Which changesets is this about, as the page links to http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e13f21ac6d83&tochange=03453271ff96 which is massive ?
Flags: needinfo?(avihpit)
Flags: needinfo?(avihpit) → needinfo?(jmaher)
Also, the summary points to http://hg.mozilla.org/integration/fx-team/pushloghtml?rev=d6f1397b3563 which has no patches by either me or margaret.

I don't think it's reasonable to threaten with backout in 3 working days when it is completely unclear which patches caused what thing and where to even begin to investigate.
odd, I see this:
https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=d6f1397b3563

it is 4 patches authored by Margaret.

On Alert manager a + (highlight by blue) is an improvement and a -x.xx is a regression.
Flags: needinfo?(jmaher)
So we improved non-PGO by 20-40% but regressed PGO by 20%?

That seems... odd.
(In reply to Joel Maher (:jmaher) from comment #2)
> Margaret, any thoughts on this?  I think this might just be a random blip in
> the way pgo works.

I most definitely do not think bug 1131004 is to blame for anything, since it was just a string change.

Bugs 1143844 and 1145259 did change some logic on page load, so maybe that's interacting poorly with the test. If there was a legitimate page load regression, I would think we would see this in other tests as well. I'm not familiar with what this test is doing in particular that might cause problems.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #7)
> ... If there was a legitimate page load
> regression, I would think we would see this in other tests as well. I'm not
> familiar with what this test is doing in particular that might cause
> problems.

No. The content of this test is one of the quickest which the pageloader measures among all the pageload tests, so any overheads which happens between the location change until the page is loaded affects the test results (in percentage) more than on other tests.

See the note at https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg-opacity
Tip:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d049a324846

Ignoring SVG:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa0a10b43fad

comparison (once this finishes):

http://compare-talos.mattn.ca/?oldRevs=4d049a324846&newRev=aa0a10b43fad&server=graphs.mozilla.org&submit=true

If this doesn't help, I'm going to just pronounce this wontfix because the tests are messed up (this should essentially no-op the code that we added before - and the code that was there before the changesets that triggered the creation of this bug! - so if there's a regression left after this then I don't know what would cause it apart from "PGO felt like it, tough!")

If it does help, it might still be wontfix (because I don't think that many people actually look at toplevel SVG docs, so optimizing for them with extra code seems like a bit of a waste).
(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #9)
> http://compare-talos.mattn.ca/?oldRevs=4d049a324846&newRev=aa0a10b43fad&server=graphs.mozilla.org&submit=true
> 
> If this doesn't help ...

It seems to me like it did help.

> If it does help, it might still be wontfix (because I don't think that many
> people actually look at toplevel SVG docs, so optimizing for them with extra
> code seems like a bit of a waste).

Like I've mentioned on comment 8 and countless times elsewhere and on the test documentation and to you on IRC, it probably has nothing to do with the test name containing the term "SVG" - unless these patches only affect performance meaningfully on documents with attributes similar to those which this test loads (which attributes are those?).

We might as well have renamed this test to "measure page load overhead (could also be affected by SVG performance)".
(In reply to Avi Halachmi (:avih) from comment #10)
> (In reply to :Gijs Kruitbosch (away 26-30/3) from comment #9)
> > http://compare-talos.mattn.ca/?oldRevs=4d049a324846&newRev=aa0a10b43fad&server=graphs.mozilla.org&submit=true
> > 
> > If this doesn't help ...
> 
> It seems to me like it did help.

I think I misinterpreted the view on that link. It's known to mix PGO and non-PGO results (like it says at the top of that page).

That's using the "new" compare-talos which does distinguish between them:
https://compare-talos.paas.mozilla.org/?oldBranch=Try&oldRevs=4d049a324846&newBranch=Try&newRev=aa0a10b43fad&submit=true

It shows that the second build which ignores non-HTML documents improves non-PGO results by 12% on Ubuntu 64 compared to the first (tip) build.

All the other tests show change smaller than 2%, possibly/probably noise because it only has a single reference run and a single test run.
(In reply to Avi Halachmi (:avih) from comment #11)
> It shows that the second build which ignores non-HTML documents improves
> non-PGO results by 12% on Ubuntu 64 compared to the first (tip) build.

Also, try builds don't run PGO (and therefore PGO results don't show up on the new compare-talos link), so we don't yet know how this change affected PGO builds.

Apologies for the spam.
(In reply to Avi Halachmi (:avih) from comment #12)
> (In reply to Avi Halachmi (:avih) from comment #11)
> > It shows that the second build which ignores non-HTML documents improves
> > non-PGO results by 12% on Ubuntu 64 compared to the first (tip) build.
> 
> Also, try builds don't run PGO (and therefore PGO results don't show up on
> the new compare-talos link), so we don't yet know how this change affected
> PGO builds.
> 
> Apologies for the spam.

Uhh... the try pushes have the "make this a PGO build" thing in it, so they should all be PGO builds.
(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #13)
> Uhh... the try pushes have the "make this a PGO build" thing in it, so they
> should all be PGO builds.

OK, I didn't realize those were pushed with the "pgo try build hack".

On this case it did indeed build and test pgo builds, but both treeherder and the new compare-talos see and report them as if they're non-pgo.

So the 12% improvement I stated earlier is on the pgo builds.
Can we make a decision here.  This is uplifting to Aurora today.
Who is responsible for making this decision? Please set NEEDINFO to the correct person when asking questions in bugzilla.
Flags: needinfo?(jmaher)
:Gijs, this is sustained at a regression for PGO and a much lower value for non-pgo.  We ship PGO, so I fully expect to see this regression show up on Aurora later today.  Can you weigh in if this is something we can fix or have to document here and let it go?
Flags: needinfo?(jmaher) → needinfo?(gijskruitbosch+bugs)
The talos info points to my attempts being a no-op, which makes sense because I messed up the patch I sent to try (needs braces around the instanceof clause). I'll repush later today if nobody beats me to it.

(In reply to Joel Maher (:jmaher) from comment #17)
> :Gijs, this is sustained at a regression for PGO and a much lower value for
> non-pgo.

Where does this later info (lower for non-pgo) come from? And is this still only on linux64 pgo? Data earlier pointed to improvements across the board except on linux64 pgo.

>  We ship PGO, so I fully expect to see this regression show up on
> Aurora later today.  Can you weigh in if this is something we can fix or
> have to document here and let it go?

I want to see the results of the try push, and it'd be interesting to see if this really did impact aurora and/or how much this is just the opposite of what happened during the initial landing of reader mode.
(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Joel Maher (:jmaher) from comment #17)
> > :Gijs, this is sustained at a regression for PGO and a much lower value for
> > non-pgo.
> 
> Where does this later info (lower for non-pgo) come from? And is this still
> only on linux64 pgo? Data earlier pointed to improvements across the board
> except on linux64 pgo.
> 
> >  We ship PGO, so I fully expect to see this regression show up on
> > Aurora later today.  Can you weigh in if this is something we can fix or
> > have to document here and let it go?
> 
> I want to see the results of the try push, and it'd be interesting to see if
> this really did impact aurora and/or how much this is just the opposite of
> what happened during the initial landing of reader mode.

Ugh, forgot the extra needinfo here.
Flags: needinfo?(jmaher)
New baseline push:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=88467fcf3ebb

New patched push:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d900bc6505



(In reply to Avi Halachmi (:avih) from comment #10)
> It probably has nothing to do with the
> test name containing the term "SVG" - unless these patches only affect
> performance meaningfully on documents with attributes similar to those which
> this test loads (which attributes are those?).
> 
> We might as well have renamed this test to "measure page load overhead
> (could also be affected by SVG performance)".

Regarding characteristics: it does seem like the SVG documents in question have pretty large DOMs, which means a getElementsByTagName call is likely to be slow(er) - and still result in "no, we shouldn't show a reader mode button here" because there'll be no paragraph (p) tags. So from that perspective, yes the document has characteristics which mean the added code will impact this test more than some of the other tests.

That still doesn't explain why we're somehow impacting linux64 so much when linux32 seems to have not cared and/or improved. This is a JS-only patch. There's no sensible reason that would happen.

We do have a lot of other, more realistic, page load tests, and no regressions were detected there (in fact, there were tp5 improvements!) and so it's not clear to me how we would classify the overall "impact on page load". Seems to me that what we added was strictly better than what we had before for HTML pages.

There is also just very little that we can reasonably do about this unless we don't care about offering reader mode everywhere, even on pages where the result is rubbish. The checks that the code use right now are very lightweight, and seem to not have affected much else.

We can look at the results of the try push and kill reader mode for SVG documents if that makes the test happy. The interesting bit will be whether that'll then regress the linux32 results again...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #20)
> That still doesn't explain why we're somehow impacting linux64 so much when
> linux32 seems to have not cared and/or improved. This is a JS-only patch.
> There's no sensible reason that would happen.

Indeed. However, it's not a matter of "Sense" as much as it's related to whatever the compiler and optimizations end up with.

> We do have a lot of other, more realistic, page load tests, and no
> regressions were detected there (in fact, there were tp5 improvements!) and
> so it's not clear to me how we would classify the overall "impact on page
> load". Seems to me that what we added was strictly better than what we had
> before for HTML pages.

These bugs are filed on regressed tests even if other tests have improved - to raise awareness to these regressions and take a conscious decision on them. The overall impact is for us (you, me, whoever is interested) to assess.

> There is also just very little that we can reasonably do about this unless
> we don't care about offering reader mode everywhere, even on pages where the
> result is rubbish. The checks that the code use right now are very
> lightweight, and seem to not have affected much else.

From the other tests results, it seems you're correct. As for if there are or aren't reasonable approaches to improve it - your call. Again, these bugs are filed to make sure regressions don't go unnoticed. It's not about never regressing anything.

> We can look at the results of the try push and kill reader mode for SVG
> documents if that makes the test happy. The interesting bit will be whether
> that'll then regress the linux32 results again...

We don't care about what the numbers are, and you could easily write code that would make all tests never regress, but that's not the point. The numbers just give us some heads up when they change. If it helps us improve the code, great, and if we decide that they're either unavoidable, or that's the best we can do right now etc etc etc, that's fine too.
Aurora seems to have taken a hit here as well:
http://graphs.mozilla.org/graph.html#tests=[[225,64,33],[225,1,33],[225,63,33],[225,52,33]]&sel=1425285151998,1427877151998&displayrange=30&datatype=geo

But unlike trunk branches, Aurora has a mix of high and low values.
Flags: needinfo?(jmaher)
(In reply to Avi Halachmi (:avih) from comment #21)
> (In reply to :Gijs Kruitbosch from comment #20)
> > We can look at the results of the try push and kill reader mode for SVG
> > documents if that makes the test happy. The interesting bit will be whether
> > that'll then regress the linux32 results again...
> 
> We don't care about what the numbers are, and you could easily write code
> that would make all tests never regress, but that's not the point. The
> numbers just give us some heads up when they change. If it helps us improve
> the code, great, and if we decide that they're either unavoidable, or that's
> the best we can do right now etc etc etc, that's fine too.

I mean, the current policies are percentage-based, and so keeping the numbers of the test low will help catch future regressions because smaller changes will trigger alerts. At a 20% increase that is not even really just theoretical.
I'm not sure which policies you refer to. The "policy" for _filing_ regression bugs are ~2% and reasonable confidence (of the regression % and of identifying the offending changeset).

The actual handling of the change in numbers if like I described above.
(In reply to Avi Halachmi (:avih) from comment #24)
> The actual handling of the change in numbers if like I described above.

s/if/is/
Attached file MozReview Request: bz://1147487/Gijs (obsolete) (deleted) — 
/r/6455 - Bug 1147487 - don't try to reader-ize non-HTML documents, r?margaret

Pull down this commit:

hg pull -r 51c25bd9d831fe971e640d6b431583aaa8db5e84 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8586723 - Flags: review?(margaret.leibovic)
So:
- at least on nightly, the regression was only on 32-bit Linux and PGO (I got confused earlier).
- the aurora regression I don't really understand. It doesn't seem to be consistent, and obviously other changes could be involved there, too.
- Considering that all the other tests and platforms didn't really budge, I don't think that this regression is a great cause for concern.
- however, ignoring SVG documents fixes the regression there, meaning we keep some of the existing sensitivity (57ms on 217ms is a large increase), and now at least if people view SVG/text files etc. in the browser, we will no longer bother checking if they're readerable.
- I moved the check into isProbablyReaderable because I imagine we still need to remove the button if it was there before... Margaret, is that right?
- this doesn't change video/image documents generally, which we might want to do. We could add another instanceof check for ImageDocument. I'm not sure how to detect video documents. Margaret, thoughts?
(In reply to :Gijs Kruitbosch from comment #27)
> So:
> - at least on nightly, the regression was only on 32-bit Linux and PGO (I
> got confused earlier).
> - the aurora regression I don't really understand. It doesn't seem to be
> consistent, and obviously other changes could be involved there, too.

There have been some other regressions on aurora related to enabling reading list (at least bug 1147554, not sure if there are others).

> - Considering that all the other tests and platforms didn't really budge, I
> don't think that this regression is a great cause for concern.
> - however, ignoring SVG documents fixes the regression there, meaning we
> keep some of the existing sensitivity (57ms on 217ms is a large increase),
> and now at least if people view SVG/text files etc. in the browser, we will
> no longer bother checking if they're readerable.
> - I moved the check into isProbablyReaderable because I imagine we still
> need to remove the button if it was there before... Margaret, is that right?

Yes, that sounds right.

> - this doesn't change video/image documents generally, which we might want
> to do. We could add another instanceof check for ImageDocument. I'm not sure
> how to detect video documents. Margaret, thoughts?

I'm not familiar with different document types. Is ImageDocument a subclass of HTMLDocument?

I don't think we'd ever want to show the button for image or video documents. Would the getElementsByTagName call have the same performance problems that it does on these SVG documents?
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

https://reviewboard.mozilla.org/r/6453/#review5351

Ship It!
Attachment #8586723 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #28)
> > - this doesn't change video/image documents generally, which we might want
> > to do. We could add another instanceof check for ImageDocument. I'm not sure
> > how to detect video documents. Margaret, thoughts?
> 
> I'm not familiar with different document types. Is ImageDocument a subclass
> of HTMLDocument?

instanceof for both when viewing a png and using the devtools returns 'true', and because a relationship in the other direction wouldn't make sense (ie HTMLDocument being a subclass of ImageDocument would be nonsensical), I presume so.


> I don't think we'd ever want to show the button for image or video
> documents. Would the getElementsByTagName call have the same performance
> problems that it does on these SVG documents?

Doubtful, because as best I can tell these are just very small HTML documents that have the image/video/audio element in them. SVG documents actually have a DOM as big as the SVG and such...
Looks like we can check for image/video documents using the mozSyntheticDocument flag. I'll update the patch later today.
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

/r/6455 - Bug 1147487 - don't try to reader-ize non-HTML documents, r?margaret

Pull down this commit:

hg pull -r b37207620656f286f5a561c3df5f23a1d6ae9126 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8586723 - Flags: review+ → review?(margaret.leibovic)
Ship it! (reviewboard hates me)
Attachment #8586723 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

https://reviewboard.mozilla.org/r/6453/#review5435

Ship It!
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Points: --- → 3
Component: General → Reader Mode
Flags: qe-verify-
Flags: in-testsuite-
Flags: firefox-backlog+
Product: Firefox → Toolkit
https://hg.mozilla.org/mozilla-central/rev/bf782ce1cb1f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
The graph still shows a performance regression after 4 pgo linux builds have occurred on fx-team:
http://graphs.mozilla.org/graph.html#tests=[[225,63,33],[225,1,33]]&sel=1425461258290,1428053258290&displayrange=30&datatype=geo

Is the plan to accept this regression and move on with other work?

Also this regression landed on v.39 which is on Aurora, so if we like this fix it should be uplifted to v.39.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Joel Maher (:jmaher) from comment #37)
> The graph still shows a performance regression after 4 pgo linux builds have
> occurred on fx-team:
> http://graphs.mozilla.org/graph.html#tests=[[225,63,33],[225,1,
> 33]]&sel=1425461258290,1428053258290&displayrange=30&datatype=geo

Am I correct to understand from the graph that it has not improved at all?
(In reply to Joel Maher (:jmaher) from comment #37)
> The graph still shows a performance regression after 4 pgo linux builds have
> occurred on fx-team:
> http://graphs.mozilla.org/graph.html#tests=[[225,63,33],[225,1,
> 33]]&sel=1425461258290,1428053258290&displayrange=30&datatype=geo
> 
> Is the plan to accept this regression and move on with other work?
> 
> Also this regression landed on v.39 which is on Aurora, so if we like this
> fix it should be uplifted to v.39.

This is highly surprising to me. This was the effect of the (pgo-enabled) try push:

http://compare-talos.mattn.ca/?oldRevs=88467fcf3ebb&newRev=66d900bc6505&server=graphs.mozilla.org&submit=true

As you can see, it showed the expected 21% reduction in SVGR-opacity.

Your graph is for inbound and m-c, though, not for fx-team, but it seems that doesn't change anything here. :-\

There was a difference between the patch that landed and the one that got trypushed... the trypushed patch did away with the async messages altogether, and this patch still has them but they'll always be false. I assumed the perf impact was because of the time taken determining that outcome, and it seems that was wrong...

That said, I don't know how much we can do to fix the messaging part of things. We do need to have the content update the browser... I can look at making the update bail early if we're updating to the same thing we updated to before, but I don't know if that will
Status: RESOLVED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
... help.
Considering that bug 1150656 (tracking reader mode regressions) shows ~4-5% tp5o regressions on several platforms, and the fact that tp5o and tsvgr_opacity are the only "pure" page load tests, the conclusion is that reader mode in general affected all page load times meaningfully.

But considering that comment 39 shows the expected improvement, maybe the graph from comment 37 does not include the fixes which landed? Joel?
Flags: needinfo?(jmaher)
Try push on tip that avoids updating the reader mode icon when that'd be a no-op:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d6f8611896


Baseline trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc823ff19e7
correct, we should have enough data points on all branches now to show some sign of this being fixed.
Flags: needinfo?(jmaher)
Joel, just to clarify, you're saying that the fixes which they landed show zero improvement and the entire original regression still shows after the fixes landed?
correct Avi!
(In reply to :Gijs Kruitbosch from comment #42)
> Try push on tip that avoids updating the reader mode icon when that'd be a
> no-op:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d6f8611896
> 
> 
> Baseline trypush:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc823ff19e7

This seems to do essentially nothing (except make a11y tests worse, though only marginally so).

Margaret, do you have ideas about what I'm missing here, in particular as to why the trypushed thing "worked" and the real patch didn't? I'm drawing a blank. :-\

(my backup reason is "messagemanager traffic" but I don't understand how that could cause such a big difference on non-e10s)
Flags: needinfo?(margaret.leibovic)

        Attachment #8589143 -
        Flags: review?(margaret.leibovic)

    
    

    
  
Comment on attachment 8589143 [details] [diff] [review]
don't bother sending reader mode updates when isArticle is false,

s/document/content.document/... new patch in a sec.

        Attachment #8589143 -
        Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)

        Attachment #8589143 -
        Flags: review?(margaret.leibovic)

    
    

    
  


  

        Attachment #8589148 -
        Flags: review?(margaret.leibovic)

    
  

        Attachment #8586723 -
        Flags: checkin+

    
    

    
  
Comment on attachment 8589148 [details] [diff] [review]
don't bother sending reader mode updates when isArticle is false,

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

Makes sense to me, let's hope it helps with the regressions!

::: browser/base/content/content.js
@@ +576,5 @@
>      let isArticle = ReaderMode.isProbablyReaderable(content.document);
> +    // Only send updates when there are articles; there's no point updating with
> +    // |false| all the time.
> +    if (isArticle) {
> +      sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: isArticle });

If you want, you could get rid of the `isArticle` local variable, since it's only neccessary for the if check (and then you can put `true` directly in the message).

::: toolkit/components/reader/ReaderMode.jsm
@@ -70,5 @@
>    isProbablyReaderable: function(doc) {
> -    // Only care about 'real' HTML documents:
> -    if (doc.mozSyntheticDocument || !(doc instanceof doc.defaultView.HTMLDocument)) {
> -      return false;
> -    }

I wonder if it would be worth keeping this here so that mobile can share it in the future. As it is, there's already a bunch of stuff copy/pasted between the desktop/mobile content.js, and it would be nice to push as much logic as possible into a shared place (I can also do some of this when I get around to writing a patch for bug 1150174).

        Attachment #8589148 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  
Perf-wise, this seems pretty good. Comparing turning off reading mode and this patch,

http://compare-talos.mattn.ca/?oldRevs=65ad50c0b584&newRev=5a2158b4efcc&server=graphs.mozilla.org&submit=true

is approximately a wash, though it slightly improves a11yr_paint and regresses svgr-opacity about 1-2%.

http://compare-talos.mattn.ca/?oldRevs=ac2685723d47&newRev=5a2158b4efcc&server=graphs.mozilla.org&submit=true

shows the expected massive 32-bit improvement, as well as the somewhat-expected-but-weird tiny 64-bit regression.

    
    

    
  
(In reply to :Margaret Leibovic from comment #52)
> Comment on attachment 8589148 [details] [diff] [review]
> don't bother sending reader mode updates when isArticle is false,
> 
> Review of attachment 8589148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense to me, let's hope it helps with the regressions!
> 
> ::: browser/base/content/content.js
> @@ +576,5 @@
> >      let isArticle = ReaderMode.isProbablyReaderable(content.document);
> > +    // Only send updates when there are articles; there's no point updating with
> > +    // |false| all the time.
> > +    if (isArticle) {
> > +      sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: isArticle });
> 
> If you want, you could get rid of the `isArticle` local variable, since it's
> only neccessary for the if check (and then you can put `true` directly in
> the message).

Done.

> ::: toolkit/components/reader/ReaderMode.jsm
> @@ -70,5 @@
> >    isProbablyReaderable: function(doc) {
> > -    // Only care about 'real' HTML documents:
> > -    if (doc.mozSyntheticDocument || !(doc instanceof doc.defaultView.HTMLDocument)) {
> > -      return false;
> > -    }
> 
> I wonder if it would be worth keeping this here so that mobile can share it
> in the future. As it is, there's already a bunch of stuff copy/pasted
> between the desktop/mobile content.js, and it would be nice to push as much
> logic as possible into a shared place (I can also do some of this when I get
> around to writing a patch for bug 1150174).

Done.

remote:   https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=587c23cdc5cf

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/587c23cdc5cf
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Blocks: 1150656

    
    

    
  
Comment on attachment 8589148 [details] [diff] [review]
don't bother sending reader mode updates when isArticle is false,

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: perf regressions
[Describe test coverage new/current, TreeHerder]: talos
[Risks and why]: low risk, makes adjustments to how often we send messages
[String/UUID change made/needed]: no

        Attachment #8589148 -
        Flags: approval-mozilla-beta?

        Attachment #8589148 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

This will need uplifting as well in order for stuff to apply (for landing, please transplant both csets).

Approval request comment:
See comment 56

        Attachment #8586723 -
        Flags: approval-mozilla-beta?

        Attachment #8586723 -
        Flags: approval-mozilla-aurora?

    
  

          status-firefox38:
          --- → affected

          status-firefox39:
          --- → affected

    
    

    
  
Comment on attachment 8589148 [details] [diff] [review]
don't bother sending reader mode updates when isArticle is false,

Approved for uplift to aurora, since this fixes a performance regression and we're still aiming to keep reader view enabled.

        Attachment #8589148 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

Approving for uplift to aurora.

        Attachment #8586723 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Comment on attachment 8589148 [details] [diff] [review]
don't bother sending reader mode updates when isArticle is false,

Should be in 38 beta 6

        Attachment #8589148 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8586723 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: potential minor perf issue, makes it hard to ensure parity between nightly and beta regarding reading mode
[Describe test coverage new/current, TreeHerder]: there are tests for reader mode
[Risks and why]: practically nonexistent; been baking on nightly for ages, no negative effects
[String/UUID change made/needed]: nope

NB: this patch never landed on beta/aurora, despite comment 57.

        Attachment #8586723 -
        Flags: approval-mozilla-release?

    
    

    
  
Comment on attachment 8586723 [details]
MozReview Request: bz://1147487/Gijs

Approved for 38.0.5 (m-r).

(In reply to :Gijs Kruitbosch from comment #63)
> NB: this patch never landed on beta/aurora, despite comment 57.

I see commit comments above. Did we miss a patch?

        Attachment #8586723 -
        Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #64)
> Comment on attachment 8586723 [details]
> MozReview Request: bz://1147487/Gijs
> 
> Approved for 38.0.5 (m-r).
> 
> (In reply to :Gijs Kruitbosch from comment #63)
> > NB: this patch never landed on beta/aurora, despite comment 57.
> 
> I see commit comments above. Did we miss a patch?

Yes, one of the two (the one I now made requests on) did not land.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Sorry about the snafu. I have it queued for Beta as well when I push there next.
https://hg.mozilla.org/releases/mozilla-release/rev/f44dff585598

          status-firefox38.0.5:
          --- → fixed

    
    

    
  

        Attachment #8586723 -
        Attachment is obsolete: true

        Attachment #8619869 -
        Flags: review+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: