Closed
Bug 1382889
Opened 7 years ago
Closed 7 years ago
Update awsy test to explicitly remove the preloaded browser for the tabs closed measurements
Categories
(Testing :: AWSY, enhancement)
Testing
AWSY
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(2 files)
3.33 KB,
patch
|
bc
:
review+
|
Details | Diff | Splinter Review |
3.60 KB,
patch
|
bc
:
review+
mconley
:
feedback+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
(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•7 years ago
|
||
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 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39a56b7af955f3150bdcd074a346398acfdea63
Comment 9•7 years ago
|
||
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+
Assignee | ||
Comment 10•7 years ago
|
||
(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?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bob)
Updated•7 years ago
|
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
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/92d9a593abda https://hg.mozilla.org/mozilla-central/rev/484096481587
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Component: General → AWSY
You need to log in
before you can comment on or make changes to this bug.
Description
•