Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 2 • 9 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 4 • 9 years ago

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 :)

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 6 • 9 years ago

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!

	Ankit Goyal Assignee
	Comment 7 • 9 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 8 • 9 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 9 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 10 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 11 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 12 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 13 • 9 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 14 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 15 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 16 • 9 years ago

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.

	William Lachance (:wlach)
	Comment 17 • 9 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 18 • 9 years ago

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

	William Lachance (:wlach)
	Comment 19 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 20 • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 21 • 7 years ago

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