1880809 - Content process freeze when opening the devtools in a project that has a lot of files in the same directory

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

A first fix has landed in bug 1879575, but I still see the issue when using my test project from the STR:

1. Clone the repository https://github.com/julienw/devtools-sort-sources-problem
2. Run npm i then npm run dev in it
3. Open http://localhost:5173 (or whatever other link displayed in the terminal)
4. Open the devtools

=> When the sources are discovered, the content process freezes during a few seconds.

Here is an updated profile: https://share.firefox.dev/3ON2K30
This clearly points towards the findIndex call that was added in bug 1879575. If I'm not wrong (but I'm bad at this), this is a O(n²) algorithm in this case. In the case of the issue we're seeing here, it looks like there are 10611 files in the same directory.

A few things could be tried:

• use findLastIndex if we expect the sources to be added in a sorted way too. Easiest but may not fix the issue in all cases.
• use a binary search algorithm, because we now the array is sorted. The sorting function would be called way less. That would be (I think) a O(nlogn) algorithm.
• batch all calls happening in the same "throttling cycle". Most difficult and error-prone because most logic needs to be rewritten. But we'd call the sort function just once. I believe this is also O(nlogn) but with less GC.

So I think I'd try a binary search implementation.

As a reminder, the issue is with the @mui/icons-material npm package:
The problem in the project is how we import icons. The MUI documentation says we should do this:

import AbcIcon from '@mui/icons-material/Abc';

But I did this instead

import { Abc as AbcIcon } from '@mui/icons-material';

As a result, I believe all of the icons-material library is loaded. In this library all files are in the same directory.

If in my project I use the recommended way to import icons, I don't have the problem anymore.

Comment 1

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Use binary search when adding sources in the source tree.

This helps address a performance issue when a bulk or sources are registered.
We could have use findIndexLast, given that the sources are often coming sorted,
but this is not always the case. For example when a new arbitrary source is created by the page.

Also, while the sources look sorted, there is no actual sort being done from the Source Map Worker,
nor by the SOURCE server resource watcher. We would need to add sorting there to be reliable.

In this patch, I'm also avoiding unecessary array in the actor codebase, and, simplifying the load source map actions.

Comment 2

[Alexandre Poirot [:ochameau] [PTO back August 21]]

A little comparison between findLastIndex versus binary search.

Binary search:
https://share.firefox.dev/42OV71Y

findLastIndex:
https://share.firefox.dev/3OKLOu8

findLastIndex is slightly faster (64ms versus 104ms for addSortedItem), but that forced to sort the sources from the SOURCE server side watcher and out of the source map loader.
Binary search is reasonably fast and will be more resilient if the incoming sources aren't coming sorted.

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

This looks great! I tried it locally and this is indeed much better!

From the profiles, I think the next thing to optimize would be this find operation: https://searchfox.org/mozilla-central/rev/97feebcab27f1a92e70ceacaa77211e9eaba0e6e/devtools/client/debugger/src/reducers/sources-tree.js#354-356
Maybe we can keep a Set of all sources?

But it's not as urgent, at least on my computer.

Comment 4

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Remove getSourceQueryString in favor of displayURL.search.

We were recomputing the query string many times from many places whereas
it was already available off hand on displayURL object.

Comment 5

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Remove useless argument on getFilename helper.

This was always refering to first argument's source in all callsites using it.
Also, at the end we don't need to call getRawSourceURL, we only need to know if this source has a URL or not.

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Compute each source displayed name only once.

We were calling this getFilename many times from many callsites.
Cache it once for all on the source object.

Also clarify what we typically display in the UI:

• we display internal ID for source without a URL,
• we strip query/search parameters,
• we decode the URI string for special characters,
• index files, i.e. files loaded without a file name, are named "(index)",
• we omit the internal ":formatted" URL prefix used for pretty printed sources.

Comment 7

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Use an explicit cached attribute on source for label displayed in SourceTree.

This help ensure that we are sorting the reducer on the same string displayed in the UI.

Comment 8

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Remove unused method from source utils.

Comment 9

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1880809 - [devtools] Decode source url encoded characters early and cache it.

This will prevent re-decoding the characters on each react update.
This will also allow other component to manipulate decoded characters.

Comment 10

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Only small improvements are reported on DAMP: 2.7% speedup on custom opening and reload:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=756d6e7fcbe0eb2772ebba34eacf770c113a9e1b&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=68ba7e6d62ff1bf2d50f43e8fdb13a8085485c68&page=1&replicates=1&showOnlyConfident=1

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66c2f0a184a1
[devtools] Remove getSourceQueryString in favor of displayURL.search. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9a676429d945
[devtools] Remove useless argument on getFilename helper. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a8803fd4fe76
[devtools] Compute each source displayed name only once. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/3a60cd70a20a
[devtools] Use an explicit cached attribute on source for label displayed in SourceTree. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/a855bafc9ad6
[devtools] Remove unused method from source utils. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/abdaaea9d96b
[devtools] Decode source url encoded characters early and cache it. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/78a9e8b17f2d
[devtools] Use binary search when adding sources in the source tree. r=devtools-reviewers,nchevobbe

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/66c2f0a184a1
https://hg.mozilla.org/mozilla-central/rev/9a676429d945
https://hg.mozilla.org/mozilla-central/rev/a8803fd4fe76
https://hg.mozilla.org/mozilla-central/rev/3a60cd70a20a
https://hg.mozilla.org/mozilla-central/rev/a855bafc9ad6
https://hg.mozilla.org/mozilla-central/rev/abdaaea9d96b
https://hg.mozilla.org/mozilla-central/rev/78a9e8b17f2d

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Alexandre Poirot [:ochameau] from comment #10)

Only small improvements are reported on DAMP: 2.7% speedup on custom opening and reload:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=756d6e7fcbe0eb2772ebba34eacf770c113a9e1b&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=68ba7e6d62ff1bf2d50f43e8fdb13a8085485c68&page=1&replicates=1&showOnlyConfident=1

2.7% is already pretty good :-)

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

== Change summary for alert #41644 (as of Sun, 03 Mar 2024 14:06:10 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
3%	damp custom.jsdebugger.open.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	726.76 -> 706.78
3%	damp custom.jsdebugger.open.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	865.52 -> 843.76

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