1773258 - Memoize correctly the selectors used by the SourceTree

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1773258 - [devtools] Merge getDisplayedSources selectors into a unique one and ensure it is memoized.

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

[Phabricator Automation]

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.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1773258 - [devtools] Avoid modifying sourcesWithUrls when sources without a URL is removed.

Comment 4

[Alexandre Poirot [:ochameau]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

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 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faf8636ac2d9
[devtools] Merge getDisplayedSources selectors into a unique one and ensure it is memoized. r=bomsy
https://hg.mozilla.org/integration/autoland/rev/04aa113b0259
[devtools] Avoid modifying sourcesWithUrls when sources without a URL is removed. r=bomsy

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/faf8636ac2d9
https://hg.mozilla.org/mozilla-central/rev/04aa113b0259

Comment 8

[Julian Descottes [:jdescottes]]

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