Closed Bug 1382889 Opened 2 years ago Closed 2 years ago

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

Categories

(Testing :: AWSY, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(2 files)

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
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
Attachment #8890023 - Flags: review?(bob)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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
Attachment #8890024 - Flags: review?(bob)
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.
Attachment #8890024 - Flags: feedback?(mconley)
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)
(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 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
Attachment #8890023 - Flags: review?(bob) → review+
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.
Attachment #8890024 - Flags: review?(bob) → review+
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.
Attachment #8890024 - Flags: feedback?(mconley) → feedback+
(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?
Flags: needinfo?(bob)
Looks fine to me.
Flags: needinfo?(bob)
Summary: Update awsy test to expliciltly remove the preloaded browser for the tabs closed measurements → Update awsy test to explicitly remove the preloaded browser for the tabs closed measurements
Blocks: 1381804
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
https://hg.mozilla.org/mozilla-central/rev/92d9a593abda
https://hg.mozilla.org/mozilla-central/rev/484096481587
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: General → AWSY
You need to log in before you can comment on or make changes to this bug.