Closed Bug 1773258 Opened 2 years ago Closed 2 years ago

Memoize correctly the selectors used by the SourceTree

Categories

(DevTools :: Debugger, enhancement)

enhancement

Tracking

(firefox103 fixed)

RESOLVED FIXED
103 Branch
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: nobody → poirot.alex

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 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.

Attachment #9280228 - Attachment is obsolete: true
Attachment #9280228 - Attachment is obsolete: false

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.

Pushed by apoirot@mozilla.com: 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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

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

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

Attachment

General

Created:
Updated:
Size: