Closed Bug 1649771 Opened 4 months ago Closed 4 months ago

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

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox-esr68 unaffected, firefox-esr78 wontfix, firefox77 unaffected, firefox78 wontfix, firefox79 fixed, firefox80 fixed)

RESOLVED FIXED
Firefox 80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- wontfix
firefox77 --- unaffected
firefox78 --- wontfix
firefox79 --- fixed
firefox80 --- fixed

People

(Reporter: jdescottes, Assigned: bomsy)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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%

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

Flags: needinfo?(hmanilla)

It could be that FinalizationRegistry calback is slowing things down.

Honza

Severity: -- → S3
Flags: needinfo?(hmanilla)
Priority: -- → P2

Reminder for me do a profile to check this.

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

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?

Regressed by: 1644275
No longer regressed by: 1646940

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

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;
  }

(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

Assignee: nobody → hmanilla
Status: NEW → ASSIGNED
Pushed by hmanilla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/caa8ef1864c2
Refactor Channel Map refSet to use a Map instead to improve perf r=Honza
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80

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.

Flags: needinfo?(hmanilla)

No need to uplift

Flags: needinfo?(hmanilla)

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:
Attachment #9162651 - Flags: approval-mozilla-beta?

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.

Attachment #9162651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.