1188638 - TPS doesn't GC/CC between test causing noisy test results

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: WIP (needs to be async to support e10s)

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 3

[Benoit Girard (:BenWa)]

This is doing bug 1184277 for TPS + fixing the load order.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8640721 [details] [diff] [review]
patch

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

::: talos/page_load_test/tabswitch/bootstrap.js
@@ +71,5 @@
>        if ((aStateFlags & loadedState) == loadedState &&
>            !aWebProgress.isLoadingDocument &&
>            aWebProgress.DOMWindow == aWebProgress.DOMWindow.top) {
> +        delete waitingToLoad[aBrowser.currentURI.spec];
> +        dump("Loaded: " + aBrowser.currentURI.spec + "\n");

left over logging?

Comment 5

[Benoit Girard (:BenWa)]

I was thinking of leaving it since it's useful. The loading phase can take 10-30 seconds and TPS just prints a bit of garbage (maybe one of the tp5 pages uses dump(str) ) during that time. This logging gives you a sense of the page load completing. It's also useful for debugging.

Let me know if you want to remove it, it's not that important.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

I don't have a strong opinion, leave it in if you think its useful.

Comment 7

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/build/talos/rev/a6529a84f68cb79e52d0318231f33d4a369583db
changeset:  a6529a84f68cb79e52d0318231f33d4a369583db
user:       Benoit Girard <[e-mail]>
date:       Wed Jul 29 16:53:34 2015 -0400
description:
Bug 1188638 - Make sure all TPS test have loaded and run GC/CC between tests. r=blassey

Comment 8

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

I had to back this out due to tps hanging:
https://hg.mozilla.org/build/talos/rev/2f01520ba82f

a log:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-814454ae718a/try-macosx64/try_yosemite_test-g2-bm106-tests1-macosx-build106.txt.gz

Comment 9

[Benoit Girard (:BenWa)]

Asking mconley since he wrote the initial patch I ported over:

What are the guarantee when from the parent I call:
aBrowser.messageManager.loadFrameScript("chrome://pageloader/content/talos-content.js", false);

and a bit later I block on:
mm.addMessageListener("Talos:ForceGC:OK", function ...);

is it possible that the child begins to load talos-content.js but doesn't have it fully loaded, then it receives the "Talos:ForceGC:OK" message before talos-content.js is ready and has it's listener attached, then having missed the message we finished loading talos-content.js?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to Benoit Girard (:BenWa) from comment #9)
> Asking mconley since he wrote the initial patch I ported over:
> 
> What are the guarantee when from the parent I call:
> aBrowser.messageManager.loadFrameScript("chrome://pageloader/content/talos-
> content.js", false);
> 
> and a bit later I block on:
> mm.addMessageListener("Talos:ForceGC:OK", function ...);
> 
> is it possible that the child begins to load talos-content.js but doesn't
> have it fully loaded, then it receives the "Talos:ForceGC:OK" message before
> talos-content.js is ready and has it's listener attached, then having missed
> the message we finished loading talos-content.js?

Messages are guaranteed to arrive in-order, and I believe when we call loadFrameScript, we're essentially just sending a single message down with the contents of the URI that you pass, and then execute it on the content side.

So it should not be possible for talos-content.js to be in a half-loaded state by the time it receives the message from the parent to start the GC.

Comment 11

[Benoit Girard (:BenWa)]

try run with the patch as-is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f877a88288
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=f1f877a88288

try run with with just the loading part:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38be77fc7828
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=38be77fc7828

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch (rebased) r=blassey

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 8647076 [details] [diff] [review]
patch (rebased) r=blassey

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

::: talos/page_load_test/tabswitch/bootstrap.js
@@ +75,5 @@
> +        delete waitingToLoad[aBrowser.currentURI.spec];
> +        dump("Loaded: " + aBrowser.currentURI.spec + "\n");
> +
> +        aBrowser.messageManager.loadFrameScript("chrome://pageloader/content/talos-content.js", false);
> +        if (Object.keys(waitingToLoad).length == 0) {

This part here must be causing the hang, the GC part is fine. I don't see the dump so I'm not sure if its not bring collected in the log or we're not loading any tab.

Comment 14

[Benoit Girard (:BenWa)]

Does the above code (and the context) look right to you (or anyone)? There's something that's causing it to work locally but not using our infrastructure.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Talked to BenWa in person.

Comment 16

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

ah, some observations by BenWa and some hacking locally and on try yields:
   let parent = localURI.spec.split(localFile.leafName)[0];
+  parent = "http://localhost/page_load_test/";

if I hack this in, my try runs are green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=409d80350c74

We have a webserver variable in talos that we could set as a preference and use to distinguish http://localhost (production), vs http://localhost:15707 (--develop mode).

There might be other ways to solve this, but this should help get this patch unblocked.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: part 0: Load from the webserver

Comment 18

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

Comment on attachment 8648145 [details] [diff] [review]
part 0: Load from the webserver

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

good stuff here!

Comment 19

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/build/talos/rev/869a812af5d26b00b5965f0b4cb52590fbf4962a
changeset:  869a812af5d26b00b5965f0b4cb52590fbf4962a
user:       Benoit Girard <[e-mail]>
date:       Fri Aug 14 16:45:49 2015 -0400
description:
Bug 1188638 - TPS should load from the webserver instead of the file scheme. r=jmaher

url:        https://hg.mozilla.org/build/talos/rev/11b48cfc4512ee44b2228d22c4466d0a79e92936
changeset:  11b48cfc4512ee44b2228d22c4466d0a79e92936
user:       Benoit Girard <[e-mail]>
date:       Wed Jul 29 16:53:34 2015 -0400
description:
Bug 1188638 - Make sure all TPS test have loaded and run GC/CC between tests. r=blassey

Comment 20

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

this was landed to inbound a few minutes ago- please close this if you feel is is done :)