1382889 - Update awsy test to explicitly remove the preloaded browser for the tabs closed measurements

Status: RESOLVED FIXED

(Testing :: AWSY, enhancement)

Description

[Eric Rahm [:erahm]]

Bug 1382746 added |gBrowser.removePreloadedBrowser()|, we should either call that before our tabs closed measurement [1] or add a new checkpoint, something like:

TabsClosed{Settled,ForceGC} and then ProcessesPurged.

I'm leaning towards just leaving the checkpoints as-is.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/testing/awsy/awsy/test_memory_usage.py#344

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Clear the preloaded process after closing tabs

Clears the preloaded process that can be kept alive by various new features so
that our 'TabsClosed' metrics continue to measure the same thing, mainly 1
chrome process and 1 content process sticking around.

MozReview-Commit-ID: L4g45o4mTzR

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Add a checkpoint between closing tabs and clearing the preloaded process

This adds an additional measurement before we clear the preloaded process. It
will allow us to track future regressions within the preloaded process.

MozReview-Commit-ID: Bza7VscEqH3

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8890024 [details] [diff] [review]
Part 2: Add a checkpoint between closing tabs and clearing the preloaded process

Mike, I've added new checkpoint 'TabsClosedExtraProcesses' for the state between closing all the tabs and clearing the preloaded process. I thought you might have an opinion on the naming.

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8890023 [details] [diff] [review]
Part 1: Clear the preloaded process after closing tabs

>+++ b/testing/awsy/awsy/test_memory_usage.py
>+            if ("removeCurrentTab" in gBrowser) {
>+                return gBrowser.removePreloadedBrowser();
This check should probably be for "removePreloadedBrowser" ? But is it needed in the first place as this test is running with the current state of mozilla-central repository? (Although I'm not familiar with this code or how it's run, so I might just be totally wrong on everything! :p)

Comment 5

[Eric Rahm [:erahm]]

(In reply to Ed Lee :Mardak from comment #4)
> Comment on attachment 8890023 [details] [diff] [review]
> Part 1: Clear the preloaded process after closing tabs
> 
> >+++ b/testing/awsy/awsy/test_memory_usage.py
> >+            if ("removeCurrentTab" in gBrowser) {
> >+                return gBrowser.removePreloadedBrowser();
> This check should probably be for "removePreloadedBrowser" ?

Oh good catch!

> But is it
> needed in the first place as this test is running with the current state of
> mozilla-central repository? (Although I'm not familiar with this code or how
> it's run, so I might just be totally wrong on everything! :p)

In general you're right, but it's useful if I'm trying to use mozregression in combination with awsy. I could go either way.

Comment 6

[Bob Clary [:bc] (inactive)]

Comment on attachment 8890023 [details] [diff] [review]
Part 1: Clear the preloaded process after closing tabs

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

Two minor nits.

::: testing/awsy/awsy/test_memory_usage.py
@@ +134,5 @@
> +              `gBrowser.removePreloadedBrowser` method.
> +        """
> +        self.logger.info("closing preloaded browser")
> +        script = """
> +            if ("removeCurrentTab" in gBrowser) {

As mardak mentioned, this should be removePreloadedBrowser.

@@ +148,5 @@
> +            self.logger.error("gBrowser.removePreloadedBrowser() JavaScript error: %s" % e)
> +        except ScriptTimeoutException:
> +            self.logger.error("gBrowser.removePreloadedBrowser() timed out")
> +        except:
> +            self.logger.error("gBrowser.removeProloadedBrowser() Unexpected error: %s" % sys.exc_info()[0])

spelling: removePreloadedBrowser

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 8890024 [details] [diff] [review]
Part 2: Add a checkpoint between closing tabs and clearing the preloaded process

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

Looks ok. I'd like to see a try run with the final patches.

Comment 8

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39a56b7af955f3150bdcd074a346398acfdea63

Comment 9

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

Comment on attachment 8890024 [details] [diff] [review]
Part 2: Add a checkpoint between closing tabs and clearing the preloaded process

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

test_memory_usage.py seems right to me! Didn't fully understand process_perf_data.py or how that all works, but I assume bc's review is enough for you on that one.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #8)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e39a56b7af955f3150bdcd074a346398acfdea63

This look good to you Bob?

Comment 11

[Bob Clary [:bc] (inactive)]

Looks fine to me.

Comment 12

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d9a593abdad1174fe96840233ebd6bca1bc1e7
Bug 1382889 - Part 1: Clear the preloaded process after closing tabs. r=bc

https://hg.mozilla.org/integration/mozilla-inbound/rev/484096481587c8c66e27a4d834ec62f596ae55f3
Bug 1382889 - Part 2: Add a checkpoint between closing tabs and clearing the preloaded process. r=bc

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92d9a593abda
https://hg.mozilla.org/mozilla-central/rev/484096481587