Closed Bug 1251732 Opened 8 years ago Closed 8 years ago

See if e10s regresses tpaint for windows opened from the parent process

Categories

(Testing :: Talos, defect)

Product:
Testing
Component:
Talos
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla47

People

(Reporter: mconley, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

MozReview Request: Bug 1251732 - Make tpaint open the window from the parent
58 bytes, text/x-review-board-request
Details 
tpaint currently measures the time it takes to open a window and draw its content. That's great, except that we live in a world where opening tabs is farrrrrrrrr and away more common. The code-paths are somewhat similar to provide a new tab to content (assuming the tab was opening by a window.open in the content).

We feel like _this_ is more important to optimize, rather than the time it takes to paint a pop-up window.

We're going to modify the current test to capture this, since introducing _new_ tests is rather difficult.
TART already captures tab animation performance. I'm less interested in capturing frame painting times for the animating times than I am capturing how long it takes for the content of a new tab to be shown to the user (as in, painted, uploaded, composited).

I'm interested in capturing two ways of opening tabs as well. One way is from the parent, where the parent opens a new tab to a particular URL. The other way is via content, which can cause a link to be opened in a new tab.
Just to be clear, this isn't about tabs versus XUL windows. It's about whether the tab/window was opened by web content (window.open) or by the user.
I got a bit of pushback on re-purposing tpaint this way from the Desktop team. The suggestion was "let's add a new subtest to TART".

jmaher, is adding subtests easier than adding entirely new tests?
Flags: needinfo?(jmaher)
subtests should be fairly easy- remember that tart is a flaky test these days (especially linux64 e10s), so I am reluctant to add more to it until we get that test cleaned up.
Flags: needinfo?(jmaher)
While probably not impossible, TART focuses explicitly on tab animation, so IMO this would be shoehorning it, at the very least semantically.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> I got a bit of pushback on re-purposing tpaint this way from the Desktop
> team.

Why?
(In reply to Avi Halachmi (:avih) from comment #5)
> While probably not impossible, TART focuses explicitly on tab animation, so
> IMO this would be shoehorning it, at the very least semantically.

I agree, semantically it doesn't match.

These are the conditions we find ourselves in:

1) Ingesting results from new Talos tests is hard
2) tpaint tests something that happens less frequently (opening windows) than other things (opening tabs)
3) TART has infrastructure for opening tabs, and it's easier to add subtests to it, since it already has subtests.
4) tpaint, while not testing the common scenario, still tests something useful.
5) Semantically, testing the time to paint the content doesn't match the original purpose of TART, whose purpose is for testing animation regressions.

Adding the test to TART gives us the new information we want, while not making us throw out data that is still useful.

So I believe that's probably our best choice given the above information.

In the long run, any subtest I add to TART should probably be spun out into its own test, once adding new tests becomes trivial.

So, here is how I'm going to proceed until further notice:

1) I'm going to do the minimal required work to make TART stable for e10s.
2) I'm going to add a new subtest that measures how long until content is painted for the two cases listed in comment 1
3) I'm going to file a bug to make ingestion of new Talos tests trivial so we never have to do this again
(In reply to Bill McCloskey (:billm) from comment #6)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> > I got a bit of pushback on re-purposing tpaint this way from the Desktop
> > team.
> 
> Why?

The reasoning was that tpaint is still giving us useful information, and that if we add the test we're talking about as a TART subtest, then we get to keep getting that useful information while getting the new stuff as well.
Filed bug 1252490 for making it easier to add new tests in the future.
Summary: Modify tpaint to measure how long it takes to open new tabs instead of windows → Add TART subtest to measure how long it takes to open new tabs instead of windows
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> (In reply to Bill McCloskey (:billm) from comment #6)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> > > I got a bit of pushback on re-purposing tpaint this way from the Desktop
> > > team.
> > 
> > Why?
> 
> The reasoning was that tpaint is still giving us useful information, and
> that if we add the test we're talking about as a TART subtest, then we get
> to keep getting that useful information while getting the new stuff as well.

I don't think this makes much sense. The fact that it's a subtest doesn't mean it doesn't affect the overall test (TART/tpaint) result.

It's true that if you specifically observed the diff between subtests, then it would show which specific tests regressed, but for the regression detection which we do regularly:

1. Only the test's overall "score" is used.
2. TART already has ~30 sub-tests, so even if this new sub-test regressed by 30% (assuming the other sub-tests were not affected), then overall TART would regress 1% due to this.
(In reply to Avi Halachmi (:avih) from comment #10)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> > (In reply to Bill McCloskey (:billm) from comment #6)
> > > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> > > > I got a bit of pushback on re-purposing tpaint this way from the Desktop
> > > > team.
> > > 
> > > Why?
> > 
> > The reasoning was that tpaint is still giving us useful information, and
> > that if we add the test we're talking about as a TART subtest, then we get
> > to keep getting that useful information while getting the new stuff as well.
> 
> I don't think this makes much sense. The fact that it's a subtest doesn't
> mean it doesn't affect the overall test (TART/tpaint) result.
> 
> It's true that if you specifically observed the diff between subtests, then
> it would show which specific tests regressed, but for the regression
> detection which we do regularly:
> 
> 1. Only the test's overall "score" is used.
> 2. TART already has ~30 sub-tests, so even if this new sub-test regressed by
> 30% (assuming the other sub-tests were not affected), then overall TART
> would regress 1% due to this.

According to jmaher, in bug 1252490 comment 1, making it so that we can easily add new Talos tests will become much simpler once we stop posting to Graph Server, and that this is likely to occur within a month's time.

So perhaps we can find a compromise:

1) Replace tpaint as part of our original plan, but don't land it. Use the measurements we get off of try to give the e10s team a sense of what improvements need to be made, and the profiles to make those improvements.
2) Once bug 1252490 is closed, turn the tpaint changes we've made (but not landed) into a new test

The problem with this compromise is that we lose out on detecting regressions that land while we're working on the problem.
A (hopefully) final twist.

After much debate in #perf, jmaher has said that adding a new Talos test is a PITA but one that he's willing to go through, at least until bug 1252490 is fixed.

So I think all parties will be satisfied if I just make a new test, and leave tpaint and TART alone.
Summary: Add TART subtest to measure how long it takes to open new tabs instead of windows → Add a new Talos test to measure how long it takes to open new tabs
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> 2) tpaint tests something that happens less frequently (opening windows)
> than other things (opening tabs)

I want to make sure we're on the same page about what this bug is about. It doesn't have anything to do with windows versus tabs. It's about whether the new window/tab is opened via window.open (which can open a new window or a new tab) or via the Ctrl-N/Ctrl-T path in Firefox.

The reason this matters is that I'm afraid the desktop team thinks we don't care about measuring the performance of opening new windows. We definitely do. It's about *how* the new window is opened--via a web page or via the user asking for it. I would be very surprised if anyone cared that much about the performance of window.open.
(In reply to Bill McCloskey (:billm) from comment #13)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> > 2) tpaint tests something that happens less frequently (opening windows)
> > than other things (opening tabs)
> 
> I want to make sure we're on the same page about what this bug is about. It
> doesn't have anything to do with windows versus tabs. It's about whether the
> new window/tab is opened via window.open (which can open a new window or a
> new tab) or via the Ctrl-N/Ctrl-T path in Firefox.

That was my understanding as well. I'm going to be writing a test that measures the time to paint and composite tab content when that tab is opened:

1) Via the parent process using gBrowser.addTab
2) Via the content process via window.open

My test will measure each of these cases individually as subtests.

Is my conception of this correct, billm? I want to make sure my conception of this is correct before I write this thing.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #13)
> I would be very surprised if anyone cared that much about the performance of window.open.

Well, as a data point, window.open calls (to open the Persona popup) can occasionally take greater than 10 seconds for me when my browser has been running for a long time (with many extensions) so I think there are existing performance issues with window.open and a performance test that can capture this issue and see if it improves/worsens would be useful.
So I talked to billm today, and I think there's been some confusion on my part. billm tried to clear this up in comment 13, but I didn't understand:

What we're thinking we want to do is alter tpaint so that we measure the time it takes to open a new window, triggered solely from the parent process.

At the very least, what we want to determine is whether or not we've somehow regressed window opening from the parent process. If we have, we suspect that the team overseeing the release of e10s will really want us to focus on that. If not, we suspect their feelings might be different.

That's not to say that we don't care about window.open from content. But we want to be sure where the regression lies and does not lie.

My hypothesis is that if tpaint opens windows from the parent process, that e10s and non-e10s will be indistinguishable. I'll close this bug once I confirm or refute that.
Summary: Add a new Talos test to measure how long it takes to open new tabs → See if e10s regresses tpaint for windows opened from the parent process
This makes a lot more sense now.

FTR tpaint.html uses window.open from a content page at https://mxr.mozilla.org/mozilla-central/source/testing/talos/talos/startup_test/tpaint.html?force=1
Do you still need info from me Mike? I'm still in favor of repurposing this test to open new windows via the Ctrl-N path.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #18)
> Do you still need info from me Mike? I'm still in favor of repurposing this
> test to open new windows via the Ctrl-N path.

Yep, all good now, thanks.
So my try push in comment 19 does a quick and dirty job of causing the parent to open the new window. I've already got data in for Linux 64, and I think it's pretty compelling (I'm not sure why Talos isnt ingesting it and showing it properly, so I've pulled the raw numbers from the logs):

non-e10s     e10s
----------------------
200.29       201.14
204.41	     203.56
199.32	     204.60
196.72	     194.00
201.66	     201.85
195.93	     197.33

Geometric mean
199.70       200.38

avih, I'm not much of a statistician, but these to me look like nowhere near the ~47% regression that's being reported on https://treeherder.allizom.org/perf.html#/e10s (note that the dashboard is displaying results for PGO builds so we can't do a direct numerical comparison).

This supports my hypothesis that, at least on Linux 64, the cost of opening a window from the parent process is roughly the same for both e10s and non-e10s, and that any regression we're seeing from the tpaint test is due to the extra machinery that's kicked off when the content process needs to open a new window.

avih, do my numbers look like a compelling argument for the above?
Flags: needinfo?(avihpit)
mconley, your try push modified tpaint to do a different test and you have determined that what tpaint is measuring is invalid as e10s is ~= non-e10s?

That might be worthwhile to investigate- maybe tpaint has invalid measurements or code to execute the test.

Looking on the graphs for pgo/opt/e10s:
https://treeherder.allizom.org/perf.html#/graphs?series=[mozilla-inbound,3e3d447aabf05aef83bd85a03525905aa238af22,1]&series=[mozilla-inbound,79fb157eb94529568f02bd94de4a1bcb707d7c62,1]&series=[mozilla-inbound,b4b21ba1c506bb8fd6161e7bd2eda6e37dbe4326,1]&series=[mozilla-inbound,e7754f5593188de8aaa57a1a85c03be0896bae04,1]


you can see a clear difference between e10s and non-e10s;  Maybe we need to fix tpaint to be useful in the browser of 2016?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #21)
> avih, do my numbers look like a compelling argument for the above?

The numbers look obviously compelling for that argument. However, there are two questions IMO:

1. Do we not care anymore about new windows opened from the content? If we still care, then we should take these numbers into account too. For lack of any usage statistics which I know of, say, 50-50 contribution to an overall result?

2. Did we drop the argument expressed elsewhere that tpaint is not really important for us (and that opening a new tab, for instance, is way more important and should possibly replace tpaint?).
Flags: needinfo?(avihpit)
(In reply to Joel Maher (:jmaher) from comment #22)
> mconley, your try push modified tpaint to do a different test and you have
> determined that what tpaint is measuring is invalid as e10s is ~= non-e10s?
> 

Not necessarily invalid, no. Opening a new window from content is still a thing that's possible - the question is, is that a case that we really care about?

(In reply to Avi Halachmi (:avih) from comment #23)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #21)
> > avih, do my numbers look like a compelling argument for the above?
> 
> The numbers look obviously compelling for that argument. However, there are
> two questions IMO:
> 
> 1. Do we not care anymore about new windows opened from the content? If we
> still care, then we should take these numbers into account too. For lack of
> any usage statistics which I know of, say, 50-50 contribution to an overall
> result?
> 

That's the question here - assuming I can prove that opening windows from the parent has not changed, do we care about a regression in performance for opening tabs from content?

> 2. Did we drop the argument expressed elsewhere that tpaint is not really
> important for us (and that opening a new tab, for instance, is way more
> important and should possibly replace tpaint?).

That was originally my understanding of this bug, but I was mistaken. This got cleared up between billm and myself yesterday, and I summarize it in comment 16.

The purpose of this bug is to determine how bad the tpaint regression is (it looks like, so far, it only affects windows opened from content), and then the question is, how much do we care about that case?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #24)
> The purpose of this bug is to determine how bad the tpaint regression is (it
> looks like, so far, it only affects windows opened from content), and then
> the question is, how much do we care about that case?

That's the best I can guess... :

> (In reply to Avi Halachmi (:avih) from comment #23)
>  >  ... For lack of
> > any usage statistics which I know of, say, 50-50 contribution to an overall
> > result?

If we have some data that new windows gets rarely get opened from content, then we could ignore it, otherwise, IMO we should care about the same between those methods.
Numbers for OS X, Windows XP and Windows 7 have come in.

It's easier to just share the spreadsheet I'm using to work with them (since again, for some reason, treeherder doesn't want to show them)

https://docs.google.com/spreadsheets/d/1jO2zlOI9AxaThZKeNx3aAoCEc1UkCL-pxGt9t3DRN1g/edit?usp=sharing

Oh - turns out Perfherder accepted it anyways. Here's the comparison against the m-c base rev:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=9da51cb4974e&newProject=try&newRevision=cbdfe176e232&framework=1

So, some additional conclusions from the above link:

1) Opening the window from the parent process instead of from content is cheaper for both e10s and non-e10s
2) Opening the window from the parent is approximately the same cost for Windows XP and Windows 7 (and Linux 64 via comment 21, despite not showing up in the comparison).
3) Opening the window from the parent might be more expensive on OS X with e10s enabled - perhaps around ~12% more expensive.

I'm going to talk to some e10s folks about this discovery to see how we want to proceed.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #26)
> 3) Opening the window from the parent might be more expensive on OS X with
> e10s enabled - perhaps around ~12% more expensive.
> 

typo: that should be 2%, not 12%.
Hey Jeff,

So here's what we've got: 

According to https://treeherder.allizom.org/perf.html#/e10s, we've got a myriad of tpaint regressions with e10s, ranging from about ~44% all the way to ~106%.

The experiment that I performed in this bug suggests that this regression only occurs when the window openings are triggered from content instead of from the parent. So, for example, if I were to click on a target="_blank" link or hit some window.open() JS, that'd hit the slow path. The experiment also seems to show that windows opened from the parent (by hitting Ctrl-N, or opening a link in my mail client or something), operate at about the same efficiency with or without e10s[1].

So the question is - do we care about the window.open / target="_blank" case where we're opening new windows instead of new tabs? Because that's what this test is measuring - and e10s seems to add something in the neighourhood of 100ms (give or take) in that case.

We just want to make sure we're working on something we truly care about optimizing here.

[1]: On OS X, there might be something like a 2% regression here, though these tests are notoriously noisy.
Flags: needinfo?(jgriffiths)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #28)
> Hey Jeff,
> 
> So here's what we've got: 
> 
> According to https://treeherder.allizom.org/perf.html#/e10s, we've got a
> myriad of tpaint regressions with e10s, ranging from about ~44% all the way
> to ~106%.
> 
> The experiment that I performed in this bug suggests that this regression
> only occurs when the window openings are triggered from content instead of
> from the parent. So, for example, if I were to click on a target="_blank"
> link or hit some window.open() JS, that'd hit the slow path. The experiment
> also seems to show that windows opened from the parent (by hitting Ctrl-N,
> or opening a link in my mail client or something), operate at about the same
> efficiency with or without e10s[1].
> 
> So the question is - do we care about the window.open / target="_blank" case
> where we're opening new windows instead of new tabs? Because that's what
> this test is measuring - and e10s seems to add something in the neighbourhood
> of 100ms (give or take) in that case.
> 
> We just want to make sure we're working on something we truly care about
> optimizing here.
> 
> [1]: On OS X, there might be something like a 2% regression here, though
> these tests are notoriously noisy.

This is really excellent. One question for you - is it a different code path between a window opened by target="_blank" and right-click -> Open in new tab ? It seems like there might be overhead to both cases but I don't know how context menus work wrt e10s.

My gut is, I don't care about target="blank" as much if right-clicking is still fast. Some web compat data might be informative, eg how many sites use target="_blank" on the Alexa top 1000? How many use window.open() ?
Flags: needinfo?(jgriffiths) → needinfo?(mconley)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #29)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #28)
> > Hey Jeff,
> > 
> > So here's what we've got: 
> > 
> > According to https://treeherder.allizom.org/perf.html#/e10s, we've got a
> > myriad of tpaint regressions with e10s, ranging from about ~44% all the way
> > to ~106%.
> > 
> > The experiment that I performed in this bug suggests that this regression
> > only occurs when the window openings are triggered from content instead of
> > from the parent. So, for example, if I were to click on a target="_blank"
> > link or hit some window.open() JS, that'd hit the slow path. The experiment
> > also seems to show that windows opened from the parent (by hitting Ctrl-N,
> > or opening a link in my mail client or something), operate at about the same
> > efficiency with or without e10s[1].
> > 
> > So the question is - do we care about the window.open / target="_blank" case
> > where we're opening new windows instead of new tabs? Because that's what
> > this test is measuring - and e10s seems to add something in the neighbourhood
> > of 100ms (give or take) in that case.
> > 
> > We just want to make sure we're working on something we truly care about
> > optimizing here.
> > 
> > [1]: On OS X, there might be something like a 2% regression here, though
> > these tests are notoriously noisy.
> 
> This is really excellent. One question for you - is it a different code path
> between a window opened by target="_blank" and right-click -> Open in new
> tab ? It seems like there might be overhead to both cases but I don't know
> how context menus work wrt e10s.

It is a different code path, yes.

Actually, we can chop this down even further - by default, target="_blank" and some window.open calls will open in new tabs instead (which might be slower with e10s, but we don't know - even though that's arguably a more common occurrence, it's not something that we currently measure with Talos).

That behaviour is controlled by a preference exposed in about:preferences (General, under Tabs: "Open new windows in a new tab instead"). The Talos profile seems to have this preference unchecked so that new windows are opened instead.

The only time we open new windows from content with this default preference set is when window.open is called with non-default features.

So:

window.open("https://www.mozilla.org", "_blank");

will open in a new tab, but:

window.open("https://www.mozilla.org", "_blank", "toolbars=no");

will open in a new window.

> 
> My gut is, I don't care about target="blank" as much if right-clicking is
> still fast. Some web compat data might be informative, eg how many sites use
> target="_blank" on the Alexa top 1000? How many use window.open() ?

Good question. Main take-aways here:

1) By default, content that uses target="_blank" will open in new tabs instead. This might also hit a slow path with e10s, but we don't measure this with Talos.
2) By default, content that uses window.open with a non-default features string will cause a window to open and hit the slow path that tpaint measures.
3) Parent requesting a window to open (via the context men "Open Link in New Window" or "Open Link in New Tab") should not be affected by the slow path.
Flags: needinfo?(mconley)
Investigation concluded. The question of whether or not opening windows from the parent process is regressed by e10s has been answered.

The answer is: "No".
Blocks: e10s-perf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
See Also: → 1253382
Blocks: 1251547
Attached the patch I wrote just in case anybody needs it in the future.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: