1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

Recently we modified talos (https://wiki.mozilla.org/Buildbot/Talos), to work with e10s (https://wiki.mozilla.org/Electrolysis).  This required modifying the pageloader addon to send messages from the addon to the page we are testing and vice versa.

What we care about for this bug is to for all tests to use the e10s message system to simplify our code.

Here is a link of pageloader:
http://hg.mozilla.org/build/talos/file/8ed7a2f27b3d/talos/pageloader/chrome/pageloader.js

There is a variable "gUseE10S", which if you search for it you can see code paths which are specific for e10s.  In general we have a contentScript (either raw code or a filename), then we call loadFrameScript with that contentScript.  Now we have code that runs in the content process.  To communicate we sendAsyncMessage back and forth.  

The good news is that all of this is setup, we just need to force use of it and remove the old unnecessary code.

Some things to watch out for:
* some tests do their own measurement, some measure the raw pageload time, some suites do both- svgx has a manifest with both: http://hg.mozilla.org/build/talos/file/8ed7a2f27b3d/talos/page_load_test/svgx/svgx.manifest
* Please test on all pageloader based tests, tscrollx, tp5o_scroll, tp5o, tp5n, tsvgx, tsvgr_opacity, tart, cart, glterrain, v8_7, dromaeo_css, tcanvasmark
* tp5o_scroll uses a separate frame script, that could be confusing
* this will adjust the numbers for tests, so deploying this will require tests for stability and understanding the number shift

Comment 1

[Ankit Goyal]

Attachment: Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

Comment 2

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

Comment on attachment 8547345 [details] [diff] [review]
Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

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

My initial description and comments were not as accurate as they should have been.  We need to leave the e10s stuff in the .py files, this patch should only affect pageloader.js (and any files required to support changes in there- probably none).

The change the pageloader are a great start.  That is the right direction and I believe a bit more work there in deleting the old non e10s code paths (removing code to manage listening for events and functions to deal with it all) should clean up the file greatly and get this closer to review status :)

Regarding the python files, we need to keep e10s flags around while we are still testing non-e10s and e10s builds with Talos.  I see this happening for at least half a year if not a full year or more.

Comment 3

[Ankit Goyal]

Attachment: Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

Comment 4

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

Comment on attachment 8547958 [details] [diff] [review]
Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

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

this is much better.  I would assert that you could remove the plPaintedCapturing calls and plWaitForPaintingCapturing.  I believe we could get rid of waitForPainted and plPainted.

One thing I would like to stress, take a look at this patch inside of bugzilla as if you were reviewing it, you will see a lot of salmon colored changed which are pure whitespace- please ensure you are not adding unnecessary whitespace :)

Comment 5

[Ankit Goyal]

Attachment: Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

Comment 6

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

Comment on attachment 8548661 [details] [diff] [review]
Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

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

this is wonderful.  Can you verify this works with:
tart
cart
tsvgx
tsvgr_opacity
tscrollx

Assuming that is fine and you don't see any other functions or variables that are not being used, lets get this up for review!

Comment 7

[Ankit Goyal]

Comment on attachment 8548661 [details] [diff] [review]
Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

All mentioned tests ran fine.

I am wondering, what if we want to test non-e10s mode as now pageloader will work in perspective of only e10s.

Comment 8

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

Comment on attachment 8548661 [details] [diff] [review]
Bug 1119780 - talos pageloader can be simplified to always use the e10s path for loading/communicating. r=jmaher

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

Thanks for this patch! This works as expected!

The only concern here is that we will adjust our numbers because the code paths are a bit different.  The good news is by taking a one time hit we can now compare e10s vs non-e10s talos runs and have a higher confidence in the results since we are testing everything side by side.

Comment 9

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

https://hg.mozilla.org/build/talos/rev/d6bcfd393641

Comment 10

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

Thanks Ankit!  Another great bug you have solved.  Do let me know if you are interested in other bugs.  There are a few other talos bugs, and we have plenty of other exciting projects to work on right now on the A*Team:
https://wiki.mozilla.org/Auto-tools/Projects/Everything#Project_Table

Comment 11

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

backed out in https://hg.mozilla.org/build/talos/rev/dae3dc33aee8 due to win7 dromaeo dom crash in pgo only as documented in bug 1124740

Comment 12

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

here is a try push with the crash:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab58ac3f94a4

Comment 13

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

I have been testing this on try, it seems that magically it is fixed.  I was able to reproduce it easily a few weeks ago by pushing a pgo build to try for windows and seeing dromaeojs fail.  retriggers would yield failure as well.

Recently I have done a few pushes after I hacked around a bit and ended up with no changes to the test.  Here is my latest push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=925ba1c1732f

I am going to land this again, and plan on deploying it early next week.

Comment 14

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

https://hg.mozilla.org/build/talos/rev/aa0a4e7b7202

Comment 15

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

https://hg.mozilla.org/build/talos/rev/7208d847f6f0

Comment 16

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

Attachment: use common path for all talos except android + dromaeo* (1.0)

Until we can get dromaeo dom on win7 to act normal and figure out how to get this running smoothly on android, lets just force the e10s path in pageloader for everything else.

this will give us comparable e10s and non-e10s numbers as we will be using message manager for non-e10s.

Comment 17

[William Lachance (:wlach)]

Comment on attachment 8585564 [details] [diff] [review]
use common path for all talos except android + dromaeo* (1.0)

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

As discussed on irc, let's make it an option to disable the e10s path.

Comment 18

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

Attachment: use common path for all talos except android + dromaeo* (2.0)

switched this to --tpdisable_e10s and is only True for the cases we specifically care about.

Comment 19

[William Lachance (:wlach)]

Comment on attachment 8585643 [details] [diff] [review]
use common path for all talos except android + dromaeo* (2.0)

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

Assuming this works (it looks like there's at least one typo), I'd be happy to see this go in.

::: talos/pageloader/chrome/pageloader.js
@@ +43,5 @@
>  var gPaintWindow = window;
>  var gPaintListener = false;
>  var loadNoCache = false;
>  var scrollTest = false;
> +var gDisablE10s = false;

typo? gDisableE10S -> gDisableE10S

Comment 20

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

https://hg.mozilla.org/build/talos/rev/306e013ee359

Comment 21

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

we did these changes in other work a few months ago.