Closed Bug 1771939 Opened 2 years ago Closed 2 years ago

Simplify a few things between SourceTree utilities and the main selector used to populate it

Categories

(DevTools :: Debugger, enhancement)

Product:
DevTools
Component:
Debugger
Type:
enhancement

Tracking

(firefox103 fixed)

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox103 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

Bug 1771939 - [devtools] Remove shown source and show source in tree when source is selected.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Remove unused sortTree module from debugger.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Remove unused debuggee classname and debuggeeUrl prop from SourceTreeItem.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Migrate from debuggeeUrl/debuggeeHost to mainThreadHost.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Remove unused defaultDomain argument from getURL.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Compute the displayURL from the selector once.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Ignore sources early in the selector instead of from updateTree API.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Only pass source instead of source *and* source.displayURL.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Move `formatTree` from production to test helpers.
(deleted), text/x-phabricator-request
Details
Bug 1771939 - [devtools] Compute SourceTree data "parts" from sources selectors instead of updateTree layer.
(deleted), text/x-phabricator-request
Details

The SourceTree is solely based on getDisplayedSources reducer:
https://searchfox.org/mozilla-central/rev/32ca4fc265150e7d3d7aa6c6abea088768cf024b/devtools/client/debugger/src/selectors/sources.js#255-296
i.e. this is dedicated to the SourceTree.

The SourceTree pass this selector data to createTree:
https://searchfox.org/mozilla-central/rev/32ca4fc265150e7d3d7aa6c6abea088768cf024b/devtools/client/debugger/src/components/PrimaryPanes/SourcesTree.js#60-64
And updateTree:
https://searchfox.org/mozilla-central/rev/32ca4fc265150e7d3d7aa6c6abea088768cf024b/devtools/client/debugger/src/components/PrimaryPanes/SourcesTree.js#132-142
Which maintains a mutable copy of the selector data used by ManagedTree/Tree to display the sources.

Ideally, the selector would craft this data directly instead of having these two layers.
In this bug I'll start to cleanup a few things and move what can easily be moved to the selector.

Before, only "shown source" would really expand the tree to show the source in tree,
whereas "highlight" would only highlight the folder that is opened
(in case the source isn't visible and folder isn't expanded).

But when we select a new shown source, we also update the selected location,
which also changes the highlight items.

It is significantly clearer to only have "selected source" and have the SourceTree
to automatically expand the tree and show the selected source.

(Also a sneaky call to .reverse() was mutating the highlightedItems props...)

The classname looks useless and so is the debugeeUrl prop.

At the end, we weren't using the url but only the host.
And "debuggee" is pretty vague, let's clarify with "main thread".

This argument is only used in getDisplayURL.

Surprisingly, all these changes have no impact on DAMP:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=8002719c0fd8ea25a7b748c1f8dcbcd97bbcaabc&originalSignature=3147964&newSignature=3130994&framework=12&selectedTimeRange=172800&page=1&showOnlyComparable=1&showOnlyImportant=1

I would have expected the many calls to getDisplayURL or any added extra computation in the selector to immediately become visible as a regression. But no.

We were using isInvalidUrl late in the process of updateTree layer to ignore sources
and prevent displaying them in the SourceTree.
But getDisplayedSources is dedicated to the SourceTree and can eagerly filter out the right sources
right away.
Also getDisplayedSourceIDs is already accepting only source with a URL,
so we don't have to recheck if the source is having a URL.

I extended mochitest test coverage to replace the removed jest tests.

This helps compute it only once per source and help simplify the updateTree layer.
We no longer have to path thread/mainThreadHost everywhere.

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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.

Blocks: 1773258

Comment on attachment 9280072 [details]
Bug 1771939 - [devtools] Merge getDisplayedSources selectors into a unique one and ensure it is memoized.

Revision D148526 was moved to bug 1773258. Setting attachment 9280072 [details] to obsolete.

Attachment #9280072 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/508011e63277 [devtools] Remove shown source and show source in tree when source is selected. r=bomsy https://hg.mozilla.org/integration/autoland/rev/36c76c6b65ba [devtools] Remove unused sortTree module from debugger. r=bomsy https://hg.mozilla.org/integration/autoland/rev/8e5007ad2e4a [devtools] Remove unused debuggee classname and debuggeeUrl prop from SourceTreeItem. r=bomsy https://hg.mozilla.org/integration/autoland/rev/bfaaa1a81fdb [devtools] Migrate from debuggeeUrl/debuggeeHost to mainThreadHost. r=bomsy https://hg.mozilla.org/integration/autoland/rev/b82a549bcf50 [devtools] Remove unused defaultDomain argument from getURL. r=bomsy https://hg.mozilla.org/integration/autoland/rev/f9dc3e01a799 [devtools] Compute the displayURL from the selector once. r=bomsy https://hg.mozilla.org/integration/autoland/rev/42d229e6862b [devtools] Ignore sources early in the selector instead of from updateTree API. r=bomsy https://hg.mozilla.org/integration/autoland/rev/94381d1b8b87 [devtools] Only pass source instead of source *and* source.displayURL. r=bomsy https://hg.mozilla.org/integration/autoland/rev/03bc06fe9208 [devtools] Move `formatTree` from production to test helpers. r=bomsy https://hg.mozilla.org/integration/autoland/rev/18133832490e [devtools] Compute SourceTree data "parts" from sources selectors instead of updateTree layer. r=bomsy
Regressions: 1773362
Attachment #9280072 - Attachment is obsolete: false

== Change summary for alert #34401 (as of Fri, 10 Jun 2022 14:00:28 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
8% damp custom.jsdebugger.project-search.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender-sw 840.85 -> 911.85
7% damp custom.jsdebugger.project-search.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender 866.23 -> 922.74
6% damp custom.jsdebugger.preview.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 526.91 -> 555.96
5% damp custom.jsdebugger.project-search.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 915.04 -> 965.13
5% damp custom.jsdebugger.project-search.DAMP windows10-64-shippable-qr e10s fission stylo webrender 915.26 -> 962.47
5% damp custom.jsdebugger.preview.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 523.07 -> 549.60
4% damp custom.jsdebugger.preview.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 495.62 -> 514.89
4% damp custom.jsdebugger.preview.DAMP windows10-64-shippable-qr e10s fission stylo webrender 498.02 -> 516.87
3% damp custom.jsdebugger.open.DAMP windows10-64-shippable-qr e10s fission stylo webrender 625.66 -> 646.16
2% damp custom.jsdebugger.reload.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 699.48 -> 714.82
2% damp custom.jsdebugger.pause.DAMP windows10-64-shippable-qr e10s fission stylo webrender 251.56 -> 256.70
2% damp custom.jsdebugger.pause.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 251.36 -> 256.48
2% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr e10s fission stylo webrender 648.86 -> 661.88
2% damp simple.jsdebugger.reload.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 293.17 -> 299.04

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
6% damp complicated.inspector.close.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender-sw 17.07 -> 15.98

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

Blocks: 1651849

Comment on attachment 9280072 [details]
Bug 1771939 - [devtools] Merge getDisplayedSources selectors into a unique one and ensure it is memoized.

Revision D148526 was moved to bug 1773258. Setting attachment 9280072 [details] to obsolete.

Attachment #9280072 - Attachment is obsolete: true

Comment on attachment 9281178 [details]
Bug 1771939 - [devtools] Avoid modifying sourcesWithUrls when sources without a URL is removed.

Revision D149235 was moved to bug 1773258. Setting attachment 9281178 [details] to obsolete.

Attachment #9281178 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: