1361140 - Keeping the browser console open slows down Firefox - _findActivityObject should use a Map

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Florian Quèze [:florian]]

My browser became janky on a very fast macbook. I noticed I had forgotten a Browser Console window on another screen. I do expect some overhead when the console is open, but the profiler shows most of the CPU spent is wasted:

- _findActivityObject dominates the profile: https://perfht.ml/2p2cjx7 45% / 1800ms of CPU spent in it. Looking at the code (http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/devtools/shared/webconsole/network-monitor.js#1180) shows this is an inefficient loop that should be replaced with a map.

- there is 237ms spent in QuerySelectorAll called from pruneOutputIfNecessary: https://perfht.ml/2p1ROke
This is the code at http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/devtools/client/webconsole/webconsole.js#2376
I think this querySelectorAll call done each time a new message is appended should be avoided, instead the code should keep an array of the nodes related to messages of the category, and just remove the first message of the category whenever a new message needs to be appended (or even better, batch these updates and remove 5 or 10 old messages at once, to reduce the amount of dom manipulations).

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> My browser became janky on a very fast macbook. I noticed I had forgotten a
> Browser Console window on another screen. I do expect some overhead when the
> console is open, but the profiler shows most of the CPU spent is wasted:
> 
> - _findActivityObject dominates the profile: https://perfht.ml/2p2cjx7 45% /
> 1800ms of CPU spent in it. Looking at the code
> (http://searchfox.org/mozilla-central/rev/
> 3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/devtools/shared/webconsole/network-
> monitor.js#1180) shows this is an inefficient loop that should be replaced
> with a map.

Thanks, I'll pull this into ongoing console work

> - there is 237ms spent in QuerySelectorAll called from
> pruneOutputIfNecessary: https://perfht.ml/2p1ROke
> This is the code at
> http://searchfox.org/mozilla-central/rev/
> 3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/devtools/client/webconsole/
> webconsole.js#2376
> I think this querySelectorAll call done each time a new message is appended
> should be avoided, instead the code should keep an array of the nodes
> related to messages of the category, and just remove the first message of
> the category whenever a new message needs to be appended (or even better,
> batch these updates and remove 5 or 10 old messages at once, to reduce the
> amount of dom manipulations).

This specific issue will be obsolete once the new console frontend is enabled in the browser console

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ah, I should have refreshed I guess...  I broke out the `_findActivityObject` part into a separate Net Monitor bug 1361149.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Ah, I should have refreshed I guess...  I broke out the
> `_findActivityObject` part into a separate Net Monitor bug 1361149.

No problem, I think your bug title was more clear anyway - I'll dupe that one to this and rename.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

In case it helps, I've got a recent-ish Foxfooding profile from this use-case, up at https://perfht.ml/2pCmEkF.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1361140 - Key openRequests and openResponses by channel instead of id for faster lookups;

Review commit: https://reviewboard.mozilla.org/r/136800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136800/

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8865139 [details]
Bug 1361140 - Key openRequests and openResponses by channel instead of id for faster lookups;

https://reviewboard.mozilla.org/r/136800/#review140072

Looks reasonable to me.

Do you have any performance data showing how much this helps?

Honza

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Comment on attachment 8865139 [details]
> Bug 1361140 - Key openRequests and openResponses by channel instead of id
> for faster lookups;
> 
> https://reviewboard.mozilla.org/r/136800/#review140072
> 
> Looks reasonable to me.
> 
> Do you have any performance data showing how much this helps?

I don't think any of our existing perf tests will cover this nicely, but this should surely cause it to drop off of the reported profile (https://perfht.ml/2p2cjx7)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38f881e9f887
Key openRequests and openResponses by channel instead of id for faster lookups;r=Honza

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38f881e9f887