Fix "g1" suite in Talos e10s

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wlach, Assigned: jmaher)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Summary: Fix "g1" suite in Talos → Fix "g1" suite in Talos e10s

Comment 1

4 years ago
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.
Depends on: 1077722

Updated

4 years ago
tracking-e10s: --- → +
(Assignee)

Comment 2

4 years ago
Created attachment 8536503 [details] [diff] [review]
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?
Assignee: avihpit → jmaher
Status: NEW → ASSIGNED
Attachment #8536503 - Flags: feedback?(avihpit)
(Assignee)

Comment 3

4 years ago
Created attachment 8537357 [details] [diff] [review]
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
Attachment #8536503 - Attachment is obsolete: true
Attachment #8536503 - Flags: feedback?(avihpit)
Attachment #8537357 - Flags: review?(avihpit)
(Assignee)

Updated

4 years ago
No longer depends on: 1077722
Blocks: 1112339

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8537971 [details] [diff] [review]
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)
Attachment #8537357 - Attachment is obsolete: true
Attachment #8537357 - Flags: review?(avihpit)
Attachment #8537971 - Flags: review?(avihpit)
(Assignee)

Comment 6

4 years ago
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

4 years ago
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.
Attachment #8537971 - Flags: review?(avihpit) → review+
(Assignee)

Updated

4 years ago
Blocks: 1113713
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.