Closed Bug 1077640 Opened 10 years ago Closed 10 years ago

Fix "g1" suite in Talos e10s

Categories

(Testing :: Talos, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: wlach, Assigned: jmaher)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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
tracking-e10s: --- → +
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)
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)
No longer depends on: 1077722
Blocks: 1112339
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.
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)
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 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+
Blocks: 1113713
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: