1649771 - DAMP Perf regression in several tests (+3% overall)

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Julian Descottes [:jdescottes]]

The patch from Bug 1644275 introduced a performance regression detected by several DAMP tests:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&newProject=try&newRevision=b66b69aad55357a396e67ed75762fd479c3a2317&originalSignature=1910071&newSignature=1910071&framework=12&originalRevision=a72101335efb5c56a807af78f8107241ae221e2c

The overall DAMP regression is 3%, with individual tests regressing up to 50%:

complicated.inspector.reload.DAMP             43.05%
complicated.jsdebugger.close.DAMP             7.87%
complicated.jsdebugger.reload.DAMP            41.82%
complicated.netmonitor.reload.DAMP            35.90%
complicated.netmonitor.requestsFinished.DAMP  22.32%
complicated.styleeditor.close.DAMP            10.52%
complicated.styleeditor.open.DAMP             5.52%
complicated.styleeditor.reload.DAMP           39.36%
complicated.webconsole.reload.DAMP            32.55%
custom.jsdebugger.reload.DAMP                 14.68%
custom.jsdebugger.stepOut.DAMP                3.98%
panelsInBackground.reload.DAMP                2.04%

Comment 1

[Julian Descottes [:jdescottes]]

Hi Bomsy, could you take a look at the regression here?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1646940

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

It could be that FinalizationRegistry calback is slowing things down.

Honza

Comment 4

[Hubert Boma Manilla (:bomsy) PTO until Sept 1st]

Reminder for me do a profile to check this.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Julian, any tips on how we could profile this?

It's likely caused by using FinalizationRegistry (GC hook)
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/server/actors/network-monitor/utils/channel-map.js#30

We don't have any platform event informing us that specific HTTP channel is no longer used, and so we rely on GC removing the channels automatically.

Hooking the GC is probably having some overhead.
I don't know if markers could help us here...

Honza

Comment 6

[Julian Descottes [:jdescottes]]

The DAMP documentation contains instructions to run DAMP tests and create a profile.
https://docs.firefox-dev.tools/tests/performance-tests.html

According to the alert, the most impacted test is complicated.inspector.reload.DAMP (>40%). With some luck, this should be significant enough to be visible locally.

./mach talos-test --activeTests damp --subtests complicated.inspector --cycles 1 --tppagecycles 1 --gecko-profile --gecko-profile-entries 100000000

Try to create one profile without your patch and one with the patch, and see if you can spot any difference.
Now, considering that the regression comes from a netmonitor actor change, we might need to modify the test slightly to first load the netmonitor actor?

Comment 7

[Julian Descottes [:jdescottes]]

I just ran the command suggested above, and channel-map shows up quite clearly in the profile: https://share.firefox.dev/3f9644R

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: channel-map-in-profile.png

Comment 9

[Julian Descottes [:jdescottes]]

It looks like the issue is with the loop in :

  getChannelById(channelId) {
    for (const ref of this.refSet) {
      const key = ref.deref();
      if (!key) {
        continue;
      }
      const { value } = this.weakMap.get(key);
      if (value.channel.channelId === channelId) {
        return value;
      }
    }
    return null;
  }

Comment 10

[Hubert Boma Manilla (:bomsy) PTO until Sept 1st]

(In reply to Julian Descottes [:jdescottes] from comment #9)

It looks like the issue is with the loop in :

  getChannelById(channelId) {
    for (const ref of this.refSet) {
      const key = ref.deref();
      if (!key) {
        continue;
      }
      const { value } = this.weakMap.get(key);
      if (value.channel.channelId === channelId) {
        return value;
      }
    }
    return null;
  }

i guess we can switch that to a Map instead of a Set

Thanks Julian

Comment 11

[Hubert Boma Manilla (:bomsy) PTO until Sept 1st]

Attachment: Bug 1649771 - Refactor Channel Map refSet to use a Map instead to improve perf r=honza

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/caa8ef1864c2
Refactor Channel Map refSet to use a Map instead to improve perf r=Honza

Comment 13

[Cristina Coroiu [:ccoroiu]]

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

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bomsy, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Hubert Boma Manilla (:bomsy) PTO until Sept 1st]

No need to uplift

Comment 16

[Hubert Boma Manilla (:bomsy) PTO until Sept 1st]

Comment on attachment 9162651 [details]
Bug 1649771 - Refactor Channel Map refSet to use a Map instead to improve perf r=honza

Beta/Release Uplift Approval Request

• User impact if declined: Perf impact on developer tools panels. Impacts only developers.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Impacts only developers. Few js changes, Improves perfomance.
• String changes made/needed:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162651 [details]
Bug 1649771 - Refactor Channel Map refSet to use a Map instead to improve perf r=honza

Devtools perf improvement. Approved for 79.0b8.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ae685a664b0