Memoize correctly the selectors used by the SourceTree
Categories
(DevTools :: Debugger, enhancement)
Tracking
(firefox103 fixed)
Tracking | Status | |
---|---|---|
firefox103 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Bug 1771939 is going to regress SourceTree performance a bit by increasing the cost of executing getDisplayedSources
selector.
But this actually highlight the fact that this selector isn't correctly memoized and always return a new Array/Object instances no matter if actual content of the source tree change or not. This is typically true when we are processing sources without URLs.
This happens to be a major source of slowness visibile in bug 1651849's investigations.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
At the end getDisplayedSources wasn't correctly memoized and generates new array instances
even if the content of the source tree doesn't change.
That's because internaly, it crafts an array which is always a new array instance.
Let's merge the two intermediate selectors into a single one so that we can more easily memoize it.
This should also help try to followup and attempt at generating the data structure needed for ManagedTree.
That, instead of generated the data for addToTree/updateTree which then feeds ManagedTree.
Comment 2•2 years ago
|
||
Comment on attachment 9280228 [details]
Bug 1773258 - [devtools] Merge getDisplayedSources selectors into a unique one and ensure it is memoized.
Revision D148526 was moved to bug 1771939. Setting attachment 9280228 [details] to obsolete.
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
Assignee | ||
Comment 4•2 years ago
|
||
Surprisingly, this patch no longer bring so many improvements:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=90778286780c2232d6b89a46dbd13bcb7e34d32b&originalSignature=4081483&newSignature=4081483&framework=12&originalRevision=b6f21836ba06f8dd4390bd7d1c874d3bb7840bfb&page=1&showOnlyConfident=1
-5% on custom.jsdebugger.preview
I'm able to see the second, final selector called less frequently thanks to memoization.
But that doesn't seem to bring a visible improvement on DAMP.
Assignee | ||
Comment 5•2 years ago
|
||
Another try run, with very last version of the patch highlight only a 2.5% improvement on the same test:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=90778286780c2232d6b89a46dbd13bcb7e34d32b&originalSignature=4081483&newSignature=4081483&framework=12&originalRevision=b6f21836ba06f8dd4390bd7d1c874d3bb7840bfb&page=1&showOnlyConfident=1
Comment 7•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/faf8636ac2d9
https://hg.mozilla.org/mozilla-central/rev/04aa113b0259
Comment 8•2 years ago
|
||
Small performance improvement on windows
== Change summary for alert #34569 (as of Sat, 18 Jun 2022 09:16:38 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
3% | damp custom.jsdebugger.preview.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender | 517.08 -> 501.73 |
3% | damp custom.jsdebugger.preview.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 514.73 -> 500.62 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34569
Description
•