1351630 - 2.4 - 32.05% sessionrestore / sessionrestore_no_auto_restore / tp5o_scroll / tpaint / tresize / ts_paint / tscrollx (linux64) regression on push 81099dbf284b1af027b99a942c2d3d0043e9b208 (Mon Mar 27 2017)

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push 81099dbf284b1af027b99a942c2d3d0043e9b208. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 32%  tresize linux64 pgo      20.48 -> 27.04
 29%  tp5o_scroll summary linux64 opt 5.67 -> 7.31
 27%  tresize linux64 opt      22.53 -> 28.57
 25%  tscrollx summary linux64 opt 5.07 -> 6.35
 23%  tp5o_scroll summary linux64 pgo 5.3 -> 6.53
 22%  tscrollx summary linux64 pgo 4.89 -> 5.94
  5%  sessionrestore linux64 opt e10s753.5 -> 790.42
  4%  sessionrestore_no_auto_restore linux64 opt 919 -> 951.42
  3%  sessionrestore_no_auto_restore linux64 opt e10s787.79 -> 814.67
  3%  sessionrestore_no_auto_restore linux64 pgo e10s648.33 -> 670.33
  3%  tpaint linux64 opt       286.18 -> 295.65
  3%  ts_paint linux64 opt     1389.67 -> 1427.08
  3%  sessionrestore linux64 pgo e10s618.5 -> 634.25
  2%  sessionrestore linux64 opt 892.29 -> 913.7

Improvements:

  3%  tp5o_scroll summary linux64 opt e10s     4.47 -> 4.35
  2%  tp5o_scroll summary linux64 pgo e10s     4.37 -> 4.27


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

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 Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

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

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Interesting. I would expect the change only improves performance. The difference is that we cache the dimension of screens earlier when program startup. That might explain why the affected tests are all startup tests. Does the test measure the app startup time?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

So it's only slow for non-e10s gtk. The reason is the patch changed nsDeviceContext::FindScreen to use Widget::GetWidgetScreen instead of ScreenForNativeWidget and it's slower for gtk because it has to query X for gdk window size.

The original nsScreenManagerGtk::ScreenForNativeWidget has optimization that if there is only one screen then just return the primary screen. I think we can do the same thing here.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1351630 - Override BaseWidget::GetWidgetScreen

Review commit: https://reviewboard.mozilla.org/r/124796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124796/

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8852611 [details]
Bug 1351630 - Override BaseWidget::GetWidgetScreen

https://reviewboard.mozilla.org/r/124796/#review127424

Talos has told us that fetching the window rect is significant.  This fix
would restore the previous behavior, but it is really taking advantage of the
test environment and targeting that specifically.

Better would be to avoid the rect query even when there are multiple monitors.

mBounds position is set to the result of GetScreenBounds() in
nsWindow::OnConfigureEvent(), and so it already contains the same values
(unless the window is being moved by and external entity, in which case there
is a race anyway).

While it may be possible to change GetScreenBounds() to avoid a query, there
may be tests depending on its timing of position changes, or the fact that it
triggers a round trip even.

The simplest optimization would be to override GetWidgetScreen() with an
implementation that uses mBounds instead of GetScreenBounds().
Are you able to try that, please?

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8852611 [details]
Bug 1351630 - Override BaseWidget::GetWidgetScreen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124796/diff/1-2/

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8852611 [details]
Bug 1351630 - Override BaseWidget::GetWidgetScreen

https://reviewboard.mozilla.org/r/124796/#review127776

::: commit-message-8d1ad:3
(Diff revision 2)
> +Bug 1351630 - Override BaseWidget::GetWidgetScreen r?karlt
> +
> +Like this? I wonder if we can simply change the base impl to use mBounds.

Yes, like this, thanks, but please explain why the method is being overridden in the commit message.

The fact that GetScreenBounds() is slow, but GetBounds() is not, when they should do the same thing for toplevel windows, is a quirk of the GTK port, and so a GTK-specific workaround seems appropriate.  For child windows, mBounds is positioned relative to the parent, and so is not correct.

::: widget/gtk/nsWindow.cpp:4901
(Diff revision 2)
> +  screenManager = do_GetService("@mozilla.org/gfx/screenmanager;1");
> +  if (!screenManager) {
> +    return nullptr;
> +  }
> +
> +  LayoutDeviceIntRect bounds = mBounds;

I hadn't thought about child windows, sorry.  I was thinking we no longer used them, and I can't see a use, but it's probably safest to add

if (!mIsTopLevel) {
  bounds.MoveTo(WidgetToScreenOffset());
}

A more efficient solution would be to return mParent->GetWidgetScreen() when !mIsTopLevel at the top of the function, but efficiency is not important if this is not used.  mParent could be null for an embedded browser.  I don't think we support that any longer, and so returning nullptr when !mIsTopLevel and !mParent would be fine.

I don't mind which solution you choose.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8852611 [details]
Bug 1351630 - Override BaseWidget::GetWidgetScreen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124796/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3cb7627e007
Override BaseWidget::GetWidgetScreen r=karlt

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a3cb7627e007

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

and a bunch of improvements here:
== Change summary for alert #5790 (as of March 31 2017 17:44 UTC) ==

Improvements:

 23%  tp5o_scroll summary linux64 opt      7.36 -> 5.64
 22%  tresize linux64 pgo                  26.08 -> 20.41
 21%  tresize linux64 opt                  29.1 -> 23.02
 21%  tscrollx summary linux64 opt         6.34 -> 5.04
 19%  tp5o_scroll summary linux64 pgo      6.54 -> 5.28
 18%  tscrollx summary linux64 pgo         5.93 -> 4.87
  9%  tscrollx summary windows8-64 opt     2.72 -> 2.46
  6%  sessionrestore linux64 opt e10s      775.29 -> 732.58
  5%  sessionrestore_no_auto_restore linux64 opt e10s802.25 -> 764.33
  4%  sessionrestore_no_auto_restore linux64 opt 941.92 -> 899.92
  4%  sessionrestore linux64 opt           895.46 -> 862.5
  4%  sessionrestore linux64 pgo e10s      632.17 -> 609.67
  3%  ts_paint linux64 opt                 1398.33 -> 1352.42
  3%  ts_paint linux64 pgo                 1176.25 -> 1145.25
  3%  ts_paint linux64 opt e10s            1284.29 -> 1250.83
  2%  damp summary linux64 opt             352.92 -> 344.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5790