1077640 - Fix "g1" suite in Talos e10s

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[William Lachance (:wlach)]

The "g1" suite ("tp5o_scroll", "glterrain") does not currently work with e10s enabled. With the latest version of talos checked out, you should be able to run them as here:

https://wiki.mozilla.org/Buildbot/Talos/Running#Testing_Locally

Just specify --e10s on the command line to make sure e10s is enabled.

Avi hinted that he wouldn't mind looking into this, assigning to him for now.

Comment 1

[Avi Halachmi (:avih)]

According to Jeff, currently WebGL is broken or mostly broken with e10s. There are plans to hopefully be fully compatible within the next few weeks. I suggest to wait with glterrain until we're there (there's no bug for that yet, but Jeff will file one).

I'll look into tp5o_scroll.

Comment 2

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

Attachment: initial stab at making tp5 scroll e10s compatible (0.5)

I did some work on this, although some comments on my approach and remaining problems would be nice.

Approach:
* remove scroll related code from pageloader and create scroll.js
* for e10s mode load scrolls.js, after receiving an onload message via e10s, send a message back to content for scrolling; when scrolls are done, send a message back with the results
* make all tp5_scroll instances run via the message bus (force e10s mode)

Issues:
* numbers for non-e10s are up 75% on linux
* profiler is not working like this
* scroll.js is not in sync now
* I am hardcoding a report function
* rAF function is only the fallback, the native one is visible from the code, but calling it results in no action

:avih, can you comment on this and let me know what you think sounds correct and what sounds incorrect?

Comment 3

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

Attachment: cleaned up a lot of details, tp5 scroll converted to e10s (1.0)

this patch cleans up a few things:
* uses content.requestAnimationFrame, doesn't check all the options
* removes a lot of dead code from tscroll.js
* ^ breaks parity with other scroll-test.js though
* numbers for linux are similar to that of before this patch
* works in e10s/non-e10s mode
* Profiler is removed from fine grain detection, designed to be pageloader level only

Comment 4

[Avi Halachmi (:avih)]

Comment on attachment 8537357 [details] [diff] [review]
cleaned up a lot of details, tp5 scroll converted to e10s (1.0)

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

Looks good, with only minor refactoring needed. Please let me review the final patch. Thanks.

::: talos/pageloader/chrome/tscroll.js
@@ +1,5 @@
> +//TODO: this is much different than the code in scroll-test.js now!!!
> +
> +// Note: The content from here upto '// End scroll test' is duplicated at:
> +//       - talos/page_load_test/scroll/scroll-test.js
> +//       - inside talos/pageloader/chrome/pageloader.js

Is it duplicated? I don't think the scroll-test.js instance uses sendAsyncMessage('PageLoader:RecordTime', msg); and instead uses tpRecordTime, and there are other differences.

I think the best solution is to make the sections indeed identical because the scroll mechanism itself is identical, and this way it will be much easier to make scroll-tests changes.

One way I could think of, is replacing sendAsyncMessage here and tpRecordTime there with a single reportResult function, and its implementation will differ between the files - after the "// End scroll test" - to keep the comment here valid and the scroll test itself identical. Same for the rAF function, where for tp5o_scroll it could be:

function rAF(fn) {
  return content.requestAnimationFrame(fn);
}

and for scroll-test.js it would be whatever it is right now - but like here, move it to after the "// End scroll test" comment.

In other words, please keep the actual scroll test sections of those files identical, because the scroll test itself _is_ identical.

Comment 5

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

Attachment: convert tp5o_scroll to work in e10s mode (1.1)

a revision from the previous patch where we tscroll.js and scroll-test.js are in parity (as much as possible)

Comment 6

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

here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=618fbb19cf54

osx 10.6 shows a regression, but the others are all fine.  NOTE: I don't have 10.8 posted, I can push again to try if you wish.

Comment 7

[Avi Halachmi (:avih)]

Comment on attachment 8537971 [details] [diff] [review]
convert tp5o_scroll to work in e10s mode (1.1)

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

Thanks.

Comment 8

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

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