1727571 - Introduce a test to track memory while reloading page with devtools opened

Status: RESOLVED FIXED

(DevTools :: General, task)

Description

[Alexandre Poirot [:ochameau]]

We are having many report of leaks while reloading pages with devtools being opened.

We know that bug 1702715 had a significant positive impact of these type of leaks, while not trigerring any positive report from any of our DAMP tests.

bug 1682212 comment 44 reports that we use 10x less memory after 20 reload of CNN webpage.

We should have a test that would have notified us about this improvement so that:

1. we do not regress this back
2. we could possibly even improve our memory usage

After some investigation in DAMP, it makes sense that it doesn't highlight improvements on long sessions with DevTools being kept opened because with reload only once per toolbox.

https://searchfox.org/mozilla-central/rev/c8ea016b87997574e1ca9127a4c370032aa6ee79/testing/talos/talos/tests/devtools/addon/content/tests/head.js#164-166
Here, we only reload once.

  dump(`Reload page on '${name}'\n`);
  let test = runTest(`${name}.reload.DAMP`);
  await damp.reloadPage(onReload);
  test.done();

And in each panel test, we reload once per toolbox:
https://searchfox.org/mozilla-central/rev/c8ea016b87997574e1ca9127a4c370032aa6ee79/testing/talos/talos/tests/devtools/addon/content/tests/inspector/complicated.js#21-23

  let toolbox = await openToolboxAndLog("complicated.inspector", "inspector");
  await reloadInspectorAndLog("complicated", toolbox);
  await closeToolboxAndLog("complicated.inspector", toolbox);

We should be reloading multiple times for a given toolbox to see if memory is freed after each reload or not. If not, the following test would probably be slower and the many reload will probably be slower as well.

Unfortunately, I quickly tried doing that, and it did not highlight any significant improvement localy when testing against custom.inspector. On top of that, DAMP was crashing the inspector when trying to reload more than once against complicated.inspector...

Comment 1

[Alexandre Poirot [:ochameau]]

I ended up reusing the logic behind this existing test:
https://searchfox.org/mozilla-central/source/devtools/client/framework/test/allocations/browser_allocations_target.js
This test is tracking the number of allocated JS objects before and after running a test script.
That test cover a very small subset of DevTools. It only track the memory usage of spawning a Commands object and fetching the first top level target.

In the upcoming patches, I'm adding two new similar test but covering:

• opening and closing of a toolbox
• reloading the webpage 10 times while having a toolbox opened

Then, I tried to compare the output of these tests without and with server targets (ie. bug 1702715):
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=22bcbe80aeea10a9a98585dde40ad6b6f9254a0a&newProject=try&newRevision=2f1df5354adc5bb97d747fc6562c0cdd770dd291&framework=12&page=1
It provides a clear and better view on regressions and improvements provided by server targets. It especially highlight the significant fix in content processes when doing page reloads.

These tests highlighted:
-95% improvement in the content process when doing 10 reloads (this wasn't reported by DAMP), but a 62% regression in the parent process (this was reported by DAMP)
-26% improvement in the parent process when instantiating a commands with the top target (=existing test)
-2% improvement when opening and closing a toolbox (=new test, testing just this one usecase)

These tests are great because:

• it is just plain mochitest, so it is much easier to run compared to DAMP
• its output seem stable enough, so that it can be run on virtual machines
• it can display the allocation sites of the objects still being allocated in order to debug and improve memory usage
• the results goes into perfdata, like DAMP and can be tracked similarly

But they also have some limitations:

• in many cases, allocation sites aren't available and so the promise of debugging isn't always true
• it record the number of allocated javascript objects, so it doesn't scale with the actual amount of memory really used. If you are leaking one JS objects holding tons of data, it will regress undercovered.
• perfdata tracking can be time consuming and requires an owner dedicated to to the triage

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1727571 - [devtools] Record allocations for toolbox opening+closing and page reloads.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1727571 - [devtools] Disable devtools testing flag in allocation tests.

This is important to disable flags.testing for many reasons:

• it helps using the production code that is being used by end users. So we are closer to a real usage of our tools.
• it prevents enabling debug code which are leaking or are explicitely doing stuff that hit performance and might allocate more objects.

We especially want to disable redux's store history feature which record all the actions,
and leads to leak tons of objects.

Comment 4

[Alexandre Poirot [:ochameau]]

Perfherder results for these two patches:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=2046c80777ca0c5f8fc4e0ee9f8dd20d6706b52a&newProject=try&newRevision=2540326933a9f34fac7bd67f059cf3ec71614a9c&framework=12&page=1
I imagine the devtools.testing is reducing the allocations.
We go from 3,804.5 down to 2,565.

Comment 5

[Alexandre Poirot [:ochameau]]

To better understand the tests and what they record, here is a summary of the impact of each leak fix to the output of these tests:
https://docs.google.com/document/d/1Jl4Qwbwi_mdWBVmNeReX-_XX_xQvhQfUntqLsi8gflU/edit#heading=h.3s7pgrvsizca
Note that these fixes are focusing on leaks happening when running the "reload" test (=browser_allocation_reload.js)

tl;dr; The leak fixes only impact "objects with stacks" and recording the "resident unique" memory doesn't highlight any improvement by any of these patches.

The resident unique is the actual amount of memory allocated by the parent process.
I wasn't expecting each individual fix to have a significant/visible impact on this.
But this is still surprising that the whole patch queue isn't having a positive impact on this.
I do fix leaking target fronts, as well as MarkupView, on each page reload.
This should be a subtantial amount of objects and memory, which should have, after a couple of reload a visible impact.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1727571 - [devtools] Prevent the allocation tracker to record its own modules.

It was tracking the special sandbox we spawn for builtin-modules.js
as well as its internal sandbox used to fetch platform globals.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1098bda4a50
[devtools] Record allocations for toolbox opening+closing and page reloads. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c9b8b52fa85b
[devtools] Prevent the allocation tracker to record its own modules. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2064d5f7e669
[devtools] Disable devtools testing flag in allocation tests. r=jdescottes

Comment 8

[Alexandre Poirot [:ochameau]]

I tried some variations of GCs, but none were really great.
The to-be-landed version is:

  const numCycles = 3;
  for (let i = 0; i < numCycles; i++) {
    Cu.forceGC();
    Cu.forceCC();
    await new Promise(resolve => Cu.schedulePreciseShrinkingGC(resolve));

    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
    await new Promise(resolve => setTimeout(resolve, 1000));
  }

That's what we have been using in DAMP so far, and AFAIK, the best approach.

Here is a comparison with the following version:

  const memSrv = Cc["@mozilla.org/memory-reporter-manager;1"]
    .getService(Ci.nsIMemoryReporterManager);
 await new Promise(resolve => {
   memSrv.minimizeMemoryUsage(resolve);
  });
 
  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
  await new Promise(resolve => setTimeout(resolve, 1000));

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=017219cde5221fd68acb5b30f52569c1e57705ef&newProject=try&newRevision=5c935d3812e5acb7e8422c5863eb59beea31459d&framework=12&page=1&filter=memory
We can see that the record are fine in the content process, where we have a low variance of only 7% in both revisions.
But the variance for parent process records are inacceptable in this version -17197% for the reload tests!

And another once with the following version:

      const memSrv = Cc["@mozilla.org/memory-reporter-manager;1"]
     .getService(Ci.nsIMemoryReporterManager);

       // In order to get stable results, we really have to do 3 GC attempts
       // *and* do wait for 1s between each GC.
       const numCycles = 3;
       for (let i = 0; i < numCycles; i++) {
        await new Promise(resolve => {
          memSrv.minimizeMemoryUsage(resolve);
        });
 
         // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
         await new Promise(resolve => setTimeout(resolve, 1000));
       }

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=017219cde5221fd68acb5b30f52569c1e57705ef&newProject=try&newRevision=1c878eb3bb6d1d8d97a54d548562542dab8e6359&framework=12&page=1&filter=memory
This is better than the previous version, but still much worse than the to-be-landed version.
422% and 724% variance on target and reload tests.

And a last flavor:

      const memSrv = Cc["@mozilla.org/memory-reporter-manager;1"]
     .getService(Ci.nsIMemoryReporterManager);

       // In order to get stable results, we really have to do 3 GC attempts
       // *and* do wait for 1s between each GC.
       const numCycles = 3;
       for (let i = 0; i < numCycles; i++) {
         Cu.forceGC();
         Cu.forceCC();
        await new Promise(resolve => {
          memSrv.minimizeMemoryUsage(resolve);
        });
 
         // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
         await new Promise(resolve => setTimeout(resolve, 1000));
       }

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=017219cde5221fd68acb5b30f52569c1e57705ef&newProject=try&newRevision=c9b0dc84421809337da8fa1bae3b22954c1328d5&framework=12&page=1&filter=memory
isn't much better with 248% and 262% on reload and target tests.

Comment 9

[Iulian Moraru]

Backed out for causing devtools failures on browser_allocation_tracker.js.

Push with failures

Failure log

Backout link

Comment 10

[Alexandre Poirot [:ochameau]]

I missed tweaking an existing to match the new API...

It should be fixed now:
https://treeherder.mozilla.org/jobs?repo=try&revision=b51e11ac94ed83d04ea3bbf2f3a3064bd5cd2d02

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2abfa9bef307
[devtools] Record allocations for toolbox opening+closing and page reloads. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b63d2379b135
[devtools] Prevent the allocation tracker to record its own modules. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9588cf22563a
[devtools] Disable devtools testing flag in allocation tests. r=jdescottes

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/2abfa9bef307
https://hg.mozilla.org/mozilla-central/rev/b63d2379b135
https://hg.mozilla.org/mozilla-central/rev/9588cf22563a