1055151 - Add tests to cap DevTools' memory footprint.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jan Keromnes [:janx]]

Currently, the Firefox Developer Tools use a lot of memory. This has several bad consequences, e.g. they are completely removed from low-memory Firefox OS devices, because they currently add a 9 MB Unique Set Size overhead to every child process.

While efforts are being made to shrink devtools memory usage (see bug 988237 and bug 989263, among others), we should add tests that ensure we keep the wins as we obtain them.

While initially relatively high, we will progressively lower the max-USS thresholds, that way we can make sure the overall devtools footprint will shrink over time.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Add tests to cap devtools memory footprint.

WIP.

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8474671 [details] [diff] [review]
Add tests to cap devtools memory footprint.

Alex, what do you think of this approach? Is a unit test the best way to do this?

If not you can stop reading here and I'll rewrite the test as something else.

Else, I couldn't figure out how to log TEST-INFO messages (appart from manually dumping them):

> 0:02.09 TEST-INFO | (xpcshell/head.js) | test init_server pending (2)
> 0:02.09 Footprint after DebuggerServer.init() is 12 kB.

For the line above I'm using `dump()`, but would need something like `info()` which doesn't exist here.

Also, success and failure messages don't work as I expect them to:

> do_check_true(footprint < max, "should not exceed " + max + " kB");

yields:

> 0:02.09 TEST-PASS | /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/head_dbg.js | [do_check_true : 378] true == true

or:

>  0:02.45 TEST-UNEXPECTED-FAIL | /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/head_dbg.js | false == true - See following stack:
> /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/head_dbg.js:do_check_true:378
> /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_memory_footprint.js:check_footprint:23
> /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_memory_footprint.js:add_browser_actors:35

none of which are helpful.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Jan Keromnes [:janx] from comment #2)
> Comment on attachment 8474671 [details] [diff] [review]
> Add tests to cap devtools memory footprint.
> 
> Alex, what do you think of this approach? Is a unit test the best way to do
> this?

I think this is a great idea. I'm just not 100% sure this is going to be a stable test.
You would need to run it multiple times on tbpl, on various OSes to confirm it stays green.
Running it as an xpcshell test should help having no extra noise coming from browser chrome, which is great!

> Else, I couldn't figure out how to log TEST-INFO messages (appart from
> manually dumping them):

What about do_print() ?

> Also, success and failure messages don't work as I expect them to:
> 
> > do_check_true(footprint < max, "should not exceed " + max + " kB");

do_check*() are deprecated in favor of ok(), equal(),...
Would they happen to also be better than do_check*() ?

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging

Comment 4

[Jan Keromnes [:janx]]

Thanks for the feedback, Alex!

> I think this is a great idea. I'm just not 100% sure this is going to be a
> stable test.
> You would need to run it multiple times on tbpl, on various OSes to confirm
> it stays green.

Indeed, just got bit by a footprint of 10016 kB after DebuggerClient.listTabs().

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=96580d7daf9a

> Running it as an xpcshell test should help having no extra noise coming from
> browser chrome, which is great!

\o/

> What about do_print() ?

`do_print()` does the job (albeit a bit verbosely):

 0:06.30 TEST-INFO | /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_memory_footprint.js | "Footprint after DebuggerClient.listTabs() is 8776 kB."

> do_check*() are deprecated in favor of ok(), equal(),...
> Would they happen to also be better than do_check*() ?
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests#Assertions_and_logging

Oh, since `info()` and `fail()` didn't work, I assumed that `ok()` didn't either.

Thanks for the reference! The output gets a bit better indeed:

 0:02.35 TEST-PASS | /c/gecko-dev/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_memory_footprint.js | [check_footprint : 23] Footprint after DebuggerClient.listTabs() is 8848 kB (should be less than 10200 kB). - true == true

Comment 5

[Jan Keromnes [:janx]]

Attachment: Add tests to cap devtools memory footprint.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8478275 [details] [diff] [review]
Add tests to cap devtools memory footprint.

Over to formal review.

Not urgent, so maybe you'd like to wait for the try results: I submitted to most platforms, and will retrigger the test a few times to make sure it's stable.

(Reminder: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=96580d7daf9a)

From your experience hunting memory devouring beasts, do you think I should add more steps with footprint checks?

Comment 7

[Jan Keromnes [:janx]]

Ha, I forgot that USS is only available in Linux builds, hence the NS_ERROR_NOT_AVAILABLE on OS X and Windows...

This test needs to be Linux-only.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8478275 [details] [diff] [review]
Add tests to cap devtools memory footprint.

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

Looks good, but it is not stable enough yet.
It failed on the second linux run:
  Footprint after DebuggerServer.init() is 164 kB (should be less than 100 kB).

I think there is a few tweaks to make it more stable and accurate.
Once tweaked, can you push to try to run xpcshell something like 5 times or more?

::: toolkit/devtools/server/tests/unit/test_memory_footprint.js
@@ +7,5 @@
> +
> +function run_test() {
> +  gMgr = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager);
> +  gRefMemory = gMgr.residentUnique;
> +

It may help to call Cu.forceGC() before running the test in order to have more stable and accurate numbers.
(If the GC kicks in during the test, we may have a negative number if previous tests allocated a lot of memory!!)

@@ +48,5 @@
> +  });
> +}
> +
> +function close_client() {
> +  gClient.close(() => run_next_test());

gClient.close(run_next_test) ?

::: toolkit/devtools/server/tests/unit/xpcshell.ini
@@ +204,5 @@
>  [test_protocolSpec.js]
>  [test_registerClient.js]
>  [test_client_request.js]
>  [test_monitor_actor.js]
> +[test_memory_footprint.js]

It would help to add:
run-sequentially = measure memory, has to be run solo

That to ensure it doesn't run in parallel with another test.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Add tests to cap devtools memory footprint.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=237a61f84af7

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8481177 [details] [diff] [review]
Add tests to cap devtools memory footprint.

Thanks for the review! I addressed all the nits, and the try looks promising.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8481177 [details] [diff] [review]
Add tests to cap devtools memory footprint.

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

Looks good!
After landing, we should watch bugs/tbpl to see if that test happens to be intermittent.
And if you haven't mentioned this to the mailing list, I would suggest you to talk about this test during the next devtools meeting.
That to explain we are watching memory consumption now and we should tweak these arbitrary numbers whenever needed.

Comment 12

[Jan Keromnes [:janx]]

Thanks!

> After landing, we should watch bugs/tbpl to see if that test happens to be
> intermittent.

Will do.

> And if you haven't mentioned this to the mailing list, I would suggest you
> to talk about this test during the next devtools meeting.
> That to explain we are watching memory consumption now and we should tweak
> these arbitrary numbers whenever needed.

Will do also.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/8dedad6ca4b3

Comment 14

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/8dedad6ca4b3

Comment 15

[Daniel Holbert [:dholbert]]

My push to add 2 new CSS properties...
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a702947b274
...appears to have pushed this test over the threshold, ~80% of the time.

So. Given the following three things...

 (a) my push just added support for 2 new relatively simple CSS properties, and is unlikely to have introduced a devtools memory regression (other than the memory required for displaying 2 more properties in devtools) (and talos will tell us if there was a memory regression detected anywhere else)

 (b) the test's threshold seems to be finely tuned to be *barely* not-passed -- per comment 4, it was 10200 KB at that point; but presumably we were already surpassing that some fraction of the time, because in the version of the test that landed, the threshold is 10500 KB.

 (c) there is discussion here of the test having a risk of being intermittent (comment 3, comment 11)

...[given those ^ things], I'm assuming that the fault lies with the test (having an arbitrary threshold that we can get pushed over), and not with the TBPL push linked baove.  (Happy to be proven wrong, though. :))

So, I'm backing out the test and reopening this bug.  (Apologies in the unlikely event that it turns out my push is actually somehow guilty...)

The backout cset is: https://hg.mozilla.org/integration/mozilla-inbound/rev/96019f3efdd1

Comment 16

[Daniel Holbert [:dholbert]]

Note that in the test failures on the TBPL link above, we're *barely* over the threshold. For example, the first two failures are:
> Footprint after DebuggerServer.addBrowserActors() is 10500 kB (should be less than 10500 kB).
> Footprint after DebuggerServer.addBrowserActors() is 10504 kB (should be less than 10500 kB).
(literally right at the threshold, and then 4kb over the threshold). And the other failures are all within ~20kb of the threshold. And there are some test-runs that passed (didn't go over the threshold), too.

I don't know about the memory characteristics of this test, but given that we're just *barely* over the threshold (and only a fraction of the time), and given that we were already pretty close to the threshold per point (b) from comment 15: it seems like my push caused a small, incremental increase in the amount of memory used by this test (which seems reasonable, when adding a new CSS property and exercising devtools).

Comment 17

[Daniel Holbert [:dholbert]]

One more data point: Tomcat filed bug 1065358 on this test sporadically failing (once) on Fx-Team, and that test-failure happened *before* my m-i push's csets (from comment 15) made it over there.

Comment 18

[Dave Camp (:dcamp)]

I agree with dholbert that we need a bit of headroom for the underlying platform.  Can we add a bit please?

Comment 19

[Daniel Holbert [:dholbert]]

Even with a bit more headroom, it's easy to imagine us hitting the new threshold when additional CSS properties are added (since apparently those increase the devtools memory usage, presumably from requiring a bit more UI to display the new properties).

Maybe it'll take a month, maybe a year, maybe 3, but at some point, we're likely to unexpectedly hit this ceiling again.  When we do, it'll be good for us to discover that memory usage has gone up, but there's not much we can do immediately at that point, since for all we know, it's just been creeping up slowly, and the push that "regresses" the test is really just the straw that breaks the camel's back.

This feels like something that would be better as a talos test -- then, we could actually see a graph of the recorded values, and we can tell whether a regression is large or small (because we can actually see what the recorded values were before it -- something that's not possible with xpcshell test). Talos is pretty good about detecting & emailing about statistically-significant regressions that it detects.

(Having said that, I've never written a talos test before, so I don't quite know what's involved. :))

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Even with a bit more headroom, it's easy to imagine us hitting the new
> threshold when additional CSS properties are added (since apparently those
> increase the devtools memory usage, presumably from requiring a bit more UI
> to display the new properties).

The test is just server side initialization and not the actual UI opening and then testing. So as far as I can see, additional CSS properties should not have caused any increase in memory and the only places we get list of all CSS properties are in the frontend.

Comment 21

[Jan Keromnes [:janx]]

Thank you Daniel for detailing the reasons of the back-out. I'll re-land the test with more headroom.

That test is important as a safety valve, and it's catastrophic that today our devtools backend uses more than 10MB of RAM. We have some efforts to fight it (e.g. bug 988237), and progressively lowering the test's thresholds will make sure we keep going in the right direction.

The measured footprint is for the backend only, so there such thing as additional UI for new CSS rules like Girish said, but what you say about the memory slowly creeping up is scary. I hope that you're wrong, but I agree that we should use Talos to characterize the footprint's trend and better catch significant regressions.

Comment 22

[Jan Keromnes [:janx]]

Attachment: Add tests to cap devtools memory footprint.

Bump addBrowserActor threshold from 10500kB to 12000kB.

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

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Even with a bit more headroom, it's easy to imagine us hitting the new
> threshold when additional CSS properties are added (since apparently those
> increase the devtools memory usage, presumably from requiring a bit more UI
> to display the new properties).
> 
> Maybe it'll take a month, maybe a year, maybe 3, but at some point, we're
> likely to unexpectedly hit this ceiling again.  When we do, it'll be good
> for us to discover that memory usage has gone up, but there's not much we
> can do immediately at that point, since for all we know, it's just been
> creeping up slowly, and the push that "regresses" the test is really just
> the straw that breaks the camel's back.
> 
> This feels like something that would be better as a talos test -- then, we
> could actually see a graph of the recorded values, and we can tell whether a
> regression is large or small (because we can actually see what the recorded
> values were before it -- something that's not possible with xpcshell test).
> Talos is pretty good about detecting & emailing about
> statistically-significant regressions that it detects.
> 
> (Having said that, I've never written a talos test before, so I don't quite
> know what's involved. :))

Huge +1. We are going to end up slowly approaching the limit - no matter what the limit is - and then it will be some poor sap who has a changeset that barely increases the footprint who gets blamed for the whole of the creep.

Comment 24

[Jan Keromnes [:janx]]

Comment on attachment 8487980 [details] [diff] [review]
Add tests to cap devtools memory footprint.

Carry over Alex's r+.

Comment 25

[Jan Keromnes [:janx]]

All green!

`checkin-needed` for test to re-land with more headroom.

`leave-open` for upcoming talos test.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f79c92446b51

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f79c92446b51

Comment 28

[Jan Keromnes [:janx]]

Comment on attachment 8487980 [details] [diff] [review]
Add tests to cap devtools memory footprint.

(This patch has landed already.)

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We shouldn't keep bugs open indefinitely that have landed changes.

A new bug can be filed for the Talos work.

Comment 30

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.