Closed Bug 1119780 Opened 5 years ago Closed 3 years ago

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

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: jmaher, Assigned: ankit.goyal90)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug][lang=python][talos_wishlist])

Attachments

(2 files, 3 obsolete files)

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 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.
Attachment #8547345 - Flags: feedback?(jmaher) → feedback-
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 :)
Attachment #8547958 - Flags: feedback?(jmaher) → feedback+
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!
Attachment #8548661 - Flags: feedback?(jmaher) → feedback+
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.
Attachment #8548661 - Flags: feedback+ → review?(jmaher)
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.
Attachment #8548661 - Flags: review?(jmaher) → review+
Assignee: nobody → ankit.goyal90
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Attachment #8585564 - Flags: review?(wlachance)
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.
Attachment #8585564 - Flags: review?(wlachance)
switched this to --tpdisable_e10s and is only True for the cases we specifically care about.
Attachment #8585564 - Attachment is obsolete: true
Attachment #8585643 - Flags: review?(wlachance)
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
Attachment #8585643 - Flags: review?(wlachance) → review+
Depends on: 1151466
Depends on: 1231626
Whiteboard: [good next bug][lang=python] → [good next bug][lang=python][talos_wishlist]
we did these changes in other work a few months ago.
Status: REOPENED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.