Closed Bug 1481423 Opened 6 years ago Closed 6 years ago

2.88 - 3.71% remote-nytimes (android-4-4-armv7-api16, android-6-0-armv8-api16) regression on push 786d4f45669071091ab246f12265b3ab87aab6ff (Fri Aug 3 2018)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
Unspecified
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- affected

People

(Reporter: igoldan, Unassigned)

References

Details

(Keywords: perf, regression) 

We have detected an autophone (Android) regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=786d4f45669071091ab246f12265b3ab87aab6ff

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  4%  remote-nytimes android-6-0-armv8-api16 opt      1,063.09 -> 1,102.55
  3%  remote-nytimes android-4-4-armv7-api16 opt      1,881.55 -> 1,935.82


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=14784

On the page above you can see an 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 jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/EngineeringProductivity/Autophone
Component: General → Layout
Product: Testing → Core
Needinfo?ing :kats, as :botond is on PTO.
Flags: needinfo?(kats)
Based on the code change I'm assuming the patch ends up layerizing an element that wasn't layerized previously, so rendering it takes a little longer. I don't think the extra layerization on pageload was intended, so this might very well be a bug. Steps to investigate would be to (a) do a try run of this test with an extra log to print out which element is getting layerized due to the new condition and (b) inspect the test nytimes page to see if it's reasonable to layerize that element given the intent of the patch.
Flags: needinfo?(kats)
Seems like it's always the <html> element that's hitting this check, and that should be getting layerized already. So there goes my theory.
Yeah this shows that the <html> element would normally return false from WantAsyncScroll() and would not get a displayport in MaybeCreateDisplayPort but with the patch it does end up getting a displayport there. So that would explain the regression. I don't know what kind of meta tag is being used on this page; if it's something that prevents zooming but produces a visual viewport that's different than the layout viewport, then this change is intended and we should accept the regression.
Do you know where I can find the page used for the nytimes-remote autophone test? It doesn't look like it lives in m-c or the autophone github repo.
Flags: needinfo?(bob)
sent oob.
Flags: needinfo?(bob)
Just to clarify the intention of the patch: the only kind of scrollable frame that it intends to layerize that wasn't layerized before, is the RCD-RSF on a page, and then only when it's (visually) scrollable. I believe that under such conditions an RCD-RSF *should* be layerized.
I think the new behavior here is correct, so this is a wontfix.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.