Update toolkit/devtools/server/tests/unit/test_memory_footprint.js to lower threshold

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 39
All
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Now that bug 1012988 landed, the memory usage of DebuggerServer is now significantly lower.
We should update toolkit/devtools/server/tests/unit/test_memory_footprint.js in order to ensure not regressing it again!
(Assignee)

Updated

3 years ago
Depends on: 1012988
(Assignee)

Updated

3 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 1

3 years ago
Arf... the test has been disabled via bug 795917 without any reason :s
(Assignee)

Comment 2

3 years ago
Created attachment 8567843 [details] [diff] [review]
patch v1

Re-enable the test and lower the thresholds.

I'm using an assertion between 2 and 3 times the typical memory consumption locally,
let's see if that replicated on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=282624176357

Comment 3

3 years ago
Comment on attachment 8567843 [details] [diff] [review]
patch v1

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

\o/ you beat me to it.

Also with "-b o -p linux64 -u xpcshell" you push to try like a sniper :)

::: toolkit/devtools/server/tests/unit/test_memory_footprint.js
@@ +30,5 @@
>  }
>  
>  function add_browser_actors() {
>    DebuggerServer.addBrowserActors();
> +  check_footprint("DebuggerServer.addBrowserActors()", 1000);

Wow, that's a very low threshold. If it works I'm amazed, looks like I missed the moment when you more than 10xed memory performance!

@@ +36,5 @@
>  
>  function connect_client() {
>    gClient = new DebuggerClient(DebuggerServer.connectPipe());
>    gClient.connect(function onConnect() {
> +    check_footprint("DebuggerClient.connect()", 1000);

No smallish bump expected after connecting?
(Assignee)

Comment 4

3 years ago
(In reply to Jan Keromnes [:janx] from comment #3)
> Comment on attachment 8567843 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8567843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> \o/ you beat me to it.
> 
> Also with "-b o -p linux64 -u xpcshell" you push to try like a sniper :)
> 
> ::: toolkit/devtools/server/tests/unit/test_memory_footprint.js
> @@ +30,5 @@
> >  }
> >  
> >  function add_browser_actors() {
> >    DebuggerServer.addBrowserActors();
> > +  check_footprint("DebuggerServer.addBrowserActors()", 1000);
> 
> Wow, that's a very low threshold. If it works I'm amazed, looks like I
> missed the moment when you more than 10xed memory performance!
> 
> @@ +36,5 @@
> >  
> >  function connect_client() {
> >    gClient = new DebuggerClient(DebuggerServer.connectPipe());
> >    gClient.connect(function onConnect() {
> > +    check_footprint("DebuggerClient.connect()", 1000);
> 
> No smallish bump expected after connecting?

I don't care that much about each step consumption as soon as the final memory used when connecting and doing listTabs is under 1MB.
We could tweak that even furthur, here is the actual numbers for a local run:
Footprint after DebuggerServer.init() is 36 kB (should be less than 500 kB).
Footprint after DebuggerServer.addBrowserActors() is 228 kB (should be less than 1000 kB).
Footprint after DebuggerClient.connect() is 316 kB (should be less than 1000 kB).
Footprint after DebuggerClient.listTabs() is 504 kB (should be less than 1500 kB).

There is two significative bumps: addBrowserActors and listTabs.
I've been quite conservative to avoid triggering the threshold by any small increase due to low level platform changes. We could be more aggresive if we add some more messages when the assertion fails in order to suggest bumping the thresholds if we hit it when working on something that has nothing to do with devtools.

Comment 5

3 years ago
Comment on attachment 8567843 [details] [diff] [review]
patch v1

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

LGTM, successfully re-triggered several times on try.

::: toolkit/devtools/server/tests/unit/test_memory_footprint.js
@@ +30,5 @@
>  }
>  
>  function add_browser_actors() {
>    DebuggerServer.addBrowserActors();
> +  check_footprint("DebuggerServer.addBrowserActors()", 1000);

Nit: Looking at your results and adding some wiggle room, I suggest we make the thresholds 500 < 800 < 1000 < 1300. If that works for you, please change this one to 800.

@@ +42,5 @@
>  }
>  
>  function list_tabs() {
>    gClient.listTabs(function onListTabs(aResponse) {
> +    check_footprint("DebuggerClient.listTabs()", 1500);

Nit: (see above) Maybe change this to 1300.
Attachment #8567843 - Flags: review+

Updated

3 years ago
OS: Windows 7 → Linux
Hardware: x86_64 → All

Comment 6

3 years ago
Bonus nit: In your patch description, s/them/thresholds/.

Comment 7

3 years ago
Nit on my nit: Suggested patch description "Bug 1134174 - Reenable devtools memory footprint assertion with lower thresholds. r=janx"
(Assignee)

Comment 8

3 years ago
Created attachment 8568569 [details] [diff] [review]
patch v2

Ok, so I lowered the thresholds to 2x the local value I'm seeing here.
Given that it is more aggresive, I've added a message to help figuring out these assertions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7153dfce907
Attachment #8567843 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
It looks like results on try are more than 2 times bigger than what I get locally...
May be because of some other win introduces in my local patch queue.
Could you give it a try to see if you get my numbers or try ones?
(Assignee)

Updated

3 years ago
Attachment #8568569 - Flags: feedback?(janx)
Comment on attachment 8568569 [details] [diff] [review]
patch v2

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

Well, these thresholds are too low. While it's tempting to go as low as possible, this test should only be a safeguard against massive regressions. For the future we might be interested in adding a talos test to actually track memory usage and catch regressions the smart way.

Also (I think I had the same issues) your `do_print` messages don't seem to appear in the try logs. Can you find another way to show these messages?

::: toolkit/devtools/server/tests/unit/test_memory_footprint.js
@@ +21,5 @@
>  function check_footprint(step, max) {
>    var footprint = (gMgr.residentUnique - gRefMemory) / 1024;
>    ok(footprint < max, "Footprint after " + step + " is " + footprint + " kB (should be less than " + max + " kB).");
> +  if (footprint >= max) {
> +    do_print("!!! Memory usage of devtool server significantly increased, or very slowly increased over time.");

Nit: "The devtools server's memory usage increased either significantly, or slowly over time."

@@ +22,5 @@
>    var footprint = (gMgr.residentUnique - gRefMemory) / 1024;
>    ok(footprint < max, "Footprint after " + step + " is " + footprint + " kB (should be less than " + max + " kB).");
> +  if (footprint >= max) {
> +    do_print("!!! Memory usage of devtool server significantly increased, or very slowly increased over time.");
> +    do_print("!!! Please very the increase with/without your patch and do not hesitate to bump the thresholds if needed.");

Nit: "If your patch doesn't cause a big increase, feel free to raise the thresholds for this test if needed."
Attachment #8568569 - Flags: feedback?(janx) → feedback-
(Assignee)

Comment 11

3 years ago
Created attachment 8572063 [details] [diff] [review]
patch v3

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

Tweaked the threshold to match master values and not my local queue.
Attachment #8568569 - Attachment is obsolete: true
Comment on attachment 8572063 [details] [diff] [review]
patch v3

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

r+ assuming green try, with still my two nits from comment 10 (maybe s/if needed/as needed/).
Attachment #8572063 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8574735 [details] [diff] [review]
patch v4

Addressed the nits, new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70c2c16b4359
Attachment #8572063 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9a7651d3fd3c
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9a7651d3fd3c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.