1761554 - `ResourceCommand`'s `_cache` can grow indefinitely

Status: REOPENED

(DevTools :: General, defect, P2)

Description

[Alex Thayer [:alexical] (she/her)]

STR:

• Go to YouTube.com
• Open up the devtools webconsole
• Navigate around a bunch, for instance bouncing from recommended video to next recommended video etc.
• Set a breakpoint here
• Notice the size of this._cache

I think we might need to forcibly expire things from this. Also it looks like it should be a Map and not an array.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Note: this looks to be resonsible for the poor performance recorded in this profile recorded by jesup: https://share.firefox.dev/3wCQWbj

Comment 2

[Alex Thayer [:alexical] (she/her)]

Of 1673 messages in a recent session,

• 645 were sources
• 477 were network-events
• 443 were network-event-stacktraces

Do we need to hold onto these indefinitely or can we cap this at some size?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I'll note when closing the console it janked for many, many seconds (I gave up waiting and used another machine for a bit)

Comment 4

[Alex Thayer [:alexical] (she/her)]

Yeah I think closing the console ends up being n^2 because for each object we want to clean up we search through the whole cache to remove the entry for it.

Comment 5

[Alexandre Poirot [:ochameau]]

This code has been introduced in bug 1637641 (there is a lot of context there and in phabricator).
We knew it was a possible perf culprit but never had time to get back to it.

Let me first reply on your current questions.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #0)

I think we might need to forcibly expire things from this. Also it looks like it should be a Map and not an array.

The fact that we use an array is to preserve the resources order, that, accross all the resource types.
We might now have strong assumption on this behavior of the ResourceCommand.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #2)

Do we need to hold onto these indefinitely or can we cap this at some size?

It would probably be specific to each resource types.
For sure, some can be capped like console and netmonitor resources, as the UI cap them anyway.
Some others like debugger sources aren't capped by the UI, but they should hopefully never reach a problematic amount!
Also, some resource types are only emitting "available" and are never updated/destroyed. They may alse receive a special treatment.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #4)

Yeah I think closing the console ends up being n^2 because for each object we want to clean up we search through the whole cache to remove the entry for it.

On tool closing we should probably avoid doing anything. So we should investigate why we do any lookup.

This internal cache of ResourceCommand relates to two distinct usecases:

• to implement resource updates/destruction, so that we retrieve the matching resource we notified on "available", by its resourceId.
• store the resources, so that if a panel is opened late, the ResourceCommand will notify these cached resources to it.

While the first usecase would probably benefits from using a Map, the second benefits from having a global array so that order is preserved.
I don't have a precise usecase in mind which justifies respecting the ordering, but I wouldn't be surprise, if we started depending on it.
Should we do like the console and use a Map while depending on the Map.values() feature that respects value insertion ordering?
We would have to be careful about one thing, we don't have strong guarantee that resourceId's are distinct between two resource types.

Regarding having some capping, we can obviously match the one already used by the UI. But this probably ought to be per resource type.

One thing that is a bit frustrating is that we end up having two caches of the same things.
One in ResourceCommand and one in each panel, typically in Redux stores (example: mutableMessagesById).
I was envisioning merging these two.
Have the ResourceCommand internal cache be directly usable as a Redux store or something, so that we have just one layer.
That may be a sizable effort, but it would simplify a few things in our redux code, while probably have some unecessary computations removed.
Could the ResourceCommand, instead of calling onAvailable/onUpdated/onDestroyed, be hooked to a dedicated Redux store and call actions for available/update/destroy. And the panel "just" use this store and actions instead of its current per-panel ones? Would the panel work fine with a pre-propulated store without the "available" actions?? That's would be my main worry.

Comment 6

[Alex Thayer [:alexical] (she/her)]

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

The fact that we use an array is to preserve the resources order, that, accross all the resource types.
We might now have strong assumption on this behavior of the ResourceCommand.

Map iterators are required to go in insertion order (MDN link), so I still think we should be using a Map, unless I'm misunderstanding?

(In reply to Doug Thayer [:dthayer] (he/him) from comment #2)

Do we need to hold onto these indefinitely or can we cap this at some size?

It would probably be specific to each resource types.
For sure, some can be capped like console and netmonitor resources, as the UI cap them anyway.
Some others like debugger sources aren't capped by the UI, but they should hopefully never reach a problematic amount!
Also, some resource types are only emitting "available" and are never updated/destroyed. They may alse receive a special treatment.

Okay, so maybe we just need to do some resource-type-specfic logic to clean these up, and then add some tests to mitigate the possibility of it growing too large in the future?

One thing that is a bit frustrating is that we end up having two caches of the same things.
One in ResourceCommand and one in each panel, typically in Redux stores (example: mutableMessagesById).
I was envisioning merging these two.
Have the ResourceCommand internal cache be directly usable as a Redux store or something, so that we have just one layer.

This seems like a good idea to me. It would save on resource usage and I suspect in the long run reduce bugs.

That may be a sizable effort, but it would simplify a few things in our redux code, while probably have some unecessary computations removed.

Do you know if any tests peek into the redux stores, or are they all just validating the final output? If the former is true I agree this sounds like quite a lift.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1761554 - Use a Map-based cache for ResourceCommand r?ochameau

This just intends to alleviate the more noticeable problem when the cache grows
too large by avoiding iterating through the array every time something changes.
There still remains the work of limiting the size of this based on what the UI
is capped at, but that can come later.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #6)

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

The fact that we use an array is to preserve the resources order, that, accross all the resource types.
We might now have strong assumption on this behavior of the ResourceCommand.

Map iterators are required to go in insertion order (MDN link), so I still think we should be using a Map, unless I'm misunderstanding?

Yes, yes, Map should work. I only discovered recently that Map.entries() preserves the insertion order.
If I knew in 2020 about that I would probably have suggested a map to be used right away in this code ;)

(In reply to Doug Thayer [:dthayer] (he/him) from comment #2)
Okay, so maybe we just need to do some resource-type-specfic logic to clean these up, and then add some tests to mitigate the possibility of it growing too large in the future?

I'm wondering if we should trim the cache on each new incoming resource, to always respect the limit.
Or if that should be done more sparsly on some particular event or user action?
If we have a unique Map it may be costly to prune the old resources.

Do you know if any tests peek into the redux stores, or are they all just validating the final output? If the former is true I agree this sounds like quite a lift.

In general, in mochitests, we rather assert the final state in React, or even the final DOM being generated.
But a couple of tests are hacking into Redux internal state:
https://searchfox.org/mozilla-central/search?q=mutableMessagesById&path=webconsole%2Ftest%2F&case=false&regexp=false
That looks quite limited.

The jest test (node folder) would probably have to be hooked up against this new, distinct redux store.
This one does extensive assertions against the redux store.

Comment 9

[Alexandre Poirot [:ochameau]]

Weird, I wanted to confirm that DAMP would report a perf improvement, but it doesn't.

While investigating, I realized that netmonitor test was disabled on damp, but this is probably the most relevant test as NETWORK_EVENT are the typical resource where we call onResourceUpdated...
So I pushed to try with this test re-enabled, but still no improvement:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=fc3d0bf85fc0d04931c10e9f1b5c412f7e4ad7c2&originalSignature=3130994&newSignature=3130994&framework=12&originalRevision=106394c190f172ff508c6ff2c9d4706433083c27&page=1&filter=netmonitor
Here is a profile for a DAMP run, we can see some stack related to onResourceUpdated, when running the console:
https://share.firefox.dev/38d1fJ5
But it is only 200ms on a 510s record.

I'll see if I can revive bug 1419327 to trigger many NETWORK_EVENT resources.

Comment 10

[Alex Thayer [:alexical] (she/her)]

This shouldn't really show an improvement for "reasonable" _cache sizes. It's only when _cache balloons up into the tens of thousands that this should be relevant. I suspect the netmonitor tests are not hitting this. The way I was able to reproduce a balooning _cache was by going to a single-page-application like YouTube and navigating around a ton.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79b0fc57aff3
Use a Map-based cache for ResourceCommand r=ochameau

Comment 12

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/79b0fc57aff3

Comment 13

[Alex Thayer [:alexical] (she/her)]

Actually, reopening this (I should have flagged as leaveopen). It's just mitigated by the Map now so it shouldn't be so painful, but the issue as names is still present.