Closed Bug 1772879 Opened 2 years ago Closed 1 year ago

Memoization in debugger selectors doesn't work

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox115 fixed)

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

The memoization added in bug 1762212 and bug 1748224 is actually not working.

export const getSourceList = createSelector(
  getSourcesMap,
  sourcesMap => {
    return [...sourcesMap.values()];
  },
  { equalityCheck: shallowEqual, resultEqualityCheck: shallowEqual }
);

Should instead be:

export const getSourceList = createSelector(
  getSourcesMap,
  sourcesMap => {
    return [...sourcesMap.values()];
  },
  {
    memoizeOptions: { equalityCheck: shallowEqual, resultEqualityCheck: shallowEqual }
  }
);

Addressing the memoization on tabs selector doesn't seem to have any impact (which may explain why we missed this):
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=47c1a1c8b65d508ac05422b1c3f87da59bd0ae84&originalSignature=4081483&newSignature=4081483&framework=12&originalRevision=79679e62855c2709162e0e4df966c2c913429990&page=1&showOnlyComparable=1&showOnlyImportant=1
(I imagine the regression on console log is some random noise, but that's weird)

Results are still pending regarding sources selector.

Looking at DAMP, fixing these memoizations doesn't seem to bring any improvement.
So let's remove this broken usage of memoizationOptions.

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Severity: -- → S3
Priority: -- → P3

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ochameau, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Flags: needinfo?(poirot.alex)
Flags: needinfo?(nchevobbe)

Alex landed lots of things around the source tree these days, not sure what's the status of this specific patch

Flags: needinfo?(nchevobbe)

DAMP isn't reliable to be confident about removing these memoizations.
I would need some significant time testing the performance IRL to be sure of the impact of the memoization.

Flags: needinfo?(poirot.alex)

createSelector argument was wrong and weren't doing shallow equal.

Attachment #9280029 - Attachment is obsolete: true
Attachment #9336981 - Attachment description: Bug 1772879 - [devtools] Correctly memoize getSourcesTabs selector. → Bug 1772879 - [devtools] Remove useless memoization of getSourcesTabs selector.
Blocks: 1836439
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cf36a746e5c6 [devtools] Correctly memoize getSelectedSourceExceptions selectors. r=devtools-reviewers,jdescottes https://hg.mozilla.org/integration/autoland/rev/fdd5ba505c91 [devtools] Remove useless memoization of getSourcesTabs selector. r=devtools-reviewers,jdescottes
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch

== Change summary for alert #38562 (as of Mon, 05 Jun 2023 07:55:30 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
32% damp custom.jsdebugger.open-large-minified-file.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 1,571.81 -> 1,072.70
32% damp custom.jsdebugger.open-large-minified-file.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 1,549.37 -> 1,060.14
10% damp custom.netmonitor.reload.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 398.63 -> 358.62
5% damp custom.jsdebugger.project-search.DAMP windows10-64-shippable-qr e10s fission stylo webrender 963.81 -> 919.63
5% damp custom.jsdebugger.project-search.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 955.02 -> 911.55

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38562

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: