Memoization in debugger selectors doesn't work
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(firefox115 fixed)
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.
Assignee | ||
Comment 1•2 years ago
|
||
It looks like for sources selector, DAMP doesn't report any improvement either:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=f5fa5e7fa91d47e133759d9aad5d269fd72694fd&originalSignature=4081483&newSignature=4081483&framework=12&originalRevision=47c1a1c8b65d508ac05422b1c3f87da59bd0ae84&page=1&showOnlyComparable=1&showOnlyConfident=1
(There is still some impact, but it also looks like some noise)
Assignee | ||
Comment 2•2 years ago
|
||
Looking at DAMP, fixing these memoizations doesn't seem to bring any improvement.
So let's remove this broken usage of memoizationOptions.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 3•2 years ago
|
||
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.
Comment 4•2 years ago
|
||
Alex landed lots of things around the source tree these days, not sure what's the status of this specific patch
Assignee | ||
Comment 5•2 years ago
|
||
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.
Assignee | ||
Comment 6•1 year ago
|
||
createSelector argument was wrong and weren't doing shallow equal.
Assignee | ||
Comment 7•1 year ago
|
||
Updated•1 year ago
|
Updated•1 year ago
|
Comment 9•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf36a746e5c6
https://hg.mozilla.org/mozilla-central/rev/fdd5ba505c91
Comment 10•1 year ago
|
||
== 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
Description
•