1374765 - Make tsvg tests hide all browser chrome

Status: RESOLVED FIXED

(Testing :: Talos, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

It seems like, at one point, this was an option (there's a tpchrome setting in the pageloader add-on). It doesn't look like most Talos tests take advantage of it.

At least for tsvg tests, I argue that interference caused by things going on in the browser UI (example, the loading throbber) just adds noise and confusion, when the purpose of the test is to measure performance and detect regressions _strictly_ within the content area.

As an example, in bug 1357093, I found out that a GPU-accelerated tab loading throbber causes a tsvgx regression because the test causes the parent and content process to both run in ASAP mode, and so the compositor gets spammed by both the parent and content, when it's really only the content performance that we care about.

So I think we should make the tsvg tests run without any browser UI. It's possible that we should make some of the other pageloader tests (I'm looking at you, tp5) behave the same way - _specifically_ because those tests care about the engine, and not about what the browser UI might happen to be doing when we load a page.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I'd say that the above is much truer now especially because we're running content outside of the parent process. We should certainly have Talos tests that measure things that happen in the parent, but tests like tsvgx should not.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Finally, while it seems like we used to support tpchrome=False, it looks like we'd need to do remoteness flipping in order to get e10s support, etc.

I think the simplest way forward would be to actually pass arguments saying what UI we want open for the browser window (Services.ww.openDialog already supports this in its features string).

Comment 3

[Mike Conley (:mconley) (:⚙️)]

For the sake of simplicity, I'll just focus on the tsvg tests in this bug, and address other tests as I find them in separate bugs.

Comment 4

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

if you have an example of how to do this and a list of tests where you believe it would be important, let me know and we can hack on it.  I see this as pretty easy to do.

FWIW, there was a feature in talos in the past, I believe called --noChrome which would launch a no chrome window:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Ftalos+nochrome&redirect=true

and this would go to pageloaer with tpchrome=False|True; this maps inside of pageloader to useBrowserChrome=True|False:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/pageloader/chrome/pageloader.js#206

possibly we have code that does this already and we just need to configure in test.py:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/test.py?q=path%3Atesting%2Ftalos%2Ftalos%2Ftest.py&redirect_type=single

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1374765 - Make the Talos pageloader support tpchrome=False with e10s enabled.

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1374765 - Make tsvg tests run without browser chrome to avoid noise from UI changes.

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

Comment 7

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

Comment on attachment 8879698 [details]
Bug 1374765 - Make the Talos pageloader support tpchrome=False with e10s enabled.

https://reviewboard.mozilla.org/r/151050/#review155920

this assumes we always use this mode which is what we have done for years on all desktop tests (all we run talos on now)- I assume a lot of numbers will change

Comment 8

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

Comment on attachment 8879699 [details]
Bug 1374765 - Make tsvg tests run without browser chrome to avoid noise from UI changes.

https://reviewboard.mozilla.org/r/151052/#review155922

this is great, please file any bugs for other tests that we should consider doing this on.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Joel Maher ( :jmaher) from comment #7)
> this assumes we always use this mode which is what we have done for years on
> all desktop tests (all we run talos on now)- I assume a lot of numbers will
> change

It turns out that we default to tpchrome = True here:

http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/testing/talos/talos/config.py#39

So getting rid of the browser UI like this will be opt-in. Re: comment 8, yeah, I'll file more bugs as I find more tests where this makes sense.

Thanks!

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879698 [details]
Bug 1374765 - Make the Talos pageloader support tpchrome=False with e10s enabled.

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8879699 [details]
Bug 1374765 - Make tsvg tests run without browser chrome to avoid noise from UI changes.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9037b354a2e8
Make the Talos pageloader support tpchrome=False with e10s enabled. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/4583ab4c0f24
Make tsvg tests run without browser chrome to avoid noise from UI changes. r=jmaher

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9037b354a2e8
https://hg.mozilla.org/mozilla-central/rev/4583ab4c0f24

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #0)
> So I think we should make the tsvg tests run without any browser UI. It's
> possible that we should make some of the other pageloader tests (I'm looking
> at you, tp5) behave the same way - _specifically_ because those tests care
> about the engine, and not about what the browser UI might happen to be doing
> when we load a page.

I don't think this is sane at all. E.g. if the throbber is more expensive and slows down pageloading in talos, it will also slow down pageloading for real users, which is really the most important measure. At the end of the day, pure engine performance is irrelevant. Another case where I just rejected a UI change that wants to knowingly regress talos is bug 1377284.

I'll needinfo you not because I think this is an issue for tsvg (it might be; I don't know enough about this test), but mainly because I'm not sure if there are other bugs on file for disabling the UI for talos, and I want you to be aware of my objection. I think I saw Ehsan express the same concern elsewhere.

Comment 15

[Dão Gottwald [:dao]]

Oops, I only wanted to CC Ehsan. Don't need any info from him.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #14)
> I don't think this is sane at all. E.g. if the throbber is more expensive
> and slows down pageloading in talos, it will also slow down pageloading for
> real users, which is really the most important measure.

Yep, and that's what's being captured by the tp5 tests, which still have the browser UI enabled.

> At the end of the  day, pure engine performance is irrelevant. Another case where
> I just rejected a UI change that wants to knowingly regress talos is bug 1377284.

I disagree, especially because jwatt owns this test, and told me over IRC that for this test, he cares exclusively about SVG rendering performance. Noise generated by the UI is superfluous and unhelpful.

> I'll needinfo you not because I think this is an issue for tsvg (it might
> be; I don't know enough about this test), but mainly because I'm not sure if
> there are other bugs on file for disabling the UI for talos, and I want you
> to be aware of my objection.

Yes, I understand. I agree that there is value in ensuring that browser UI changes don't affect page load times, which is why the browser UI is still on for the tp5 tests, which simulate loading pages from the tp5 set, and (as far as I understand) aren't meant to measure one specific part of platform.

So I must respectfully disagree with your opinion that turning off the UI for tsvgx is not sane, and think we should stay the course on this particular test.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #16)
> (In reply to Dão Gottwald [::dao] from comment #14)
> > I don't think this is sane at all. E.g. if the throbber is more expensive
> > and slows down pageloading in talos, it will also slow down pageloading for
> > real users, which is really the most important measure.
> 
> Yep, and that's what's being captured by the tp5 tests, which still have the
> browser UI enabled.
> 
> > At the end of the  day, pure engine performance is irrelevant. Another case where
> > I just rejected a UI change that wants to knowingly regress talos is bug 1377284.
> 
> I disagree, especially because jwatt owns this test, and told me over IRC
> that for this test, he cares exclusively about SVG rendering performance.
> Noise generated by the UI is superfluous and unhelpful.

I rejected that patch because no explanation was given beyond "oh we probably don't care about the UI in talos since we turned it off in other tests, so let's turn it off here too."

> > I'll needinfo you not because I think this is an issue for tsvg (it might
> > be; I don't know enough about this test), but mainly because I'm not sure if
> > there are other bugs on file for disabling the UI for talos, and I want you
> > to be aware of my objection.
> 
> Yes, I understand. I agree that there is value in ensuring that browser UI
> changes don't affect page load times, which is why the browser UI is still
> on for the tp5 tests, which simulate loading pages from the tp5 set, and (as
> far as I understand) aren't meant to measure one specific part of platform.
> 
> So I must respectfully disagree with your opinion that turning off the UI
> for tsvgx is not sane, and think we should stay the course on this
> particular test.

I was replying to your "It's possible that we should make some of the other pageloader tests (I'm looking at you, tp5) behave the same way" remark, and I explicitly said I don't know enough about tsvg to say if this was a mistake.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

So we're clear, this bug does not require manual QA testing.