Open Bug 1555498 Opened 8 months ago Updated 4 months ago

Slow Inspector opening because of collecting event listeners on DOM-heavy pages

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: Harald, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

What were you doing?

  1. Right-click & inspect element on an amazon product page

What happened?

Delay in opening Inspector (~0.7s) and further delay in focussing on the right node (~0.6s).

What should have happened?

Perceived instant opening.

Anything else we should know?

http://bit.ly/2KaGzTW – Inspector causes a 368ms hang on the content thread that blocks Inspector to just idle and not jump to the right element. hasEventListeners makes up 187ms of that time. Since events are only used for the badges, maybe this work can be done lazily.

http://bit.ly/2KevWiLisSkippedNode causes an expensive reflow because of nodeHasSize.

scrollIntoViewIfNeeded and maybeNavigateToNewSelection both cause reflows – 20ms each, so not too expensive, but right after each other.

I wasn't able to reproduce this on any elements I chose on various Amazon pages. Are you inspecting a particular type of node? Maybe a video would be helpful.

Flags: needinfo?(hkirschner)

Are you inspecting a particular type of node?

I inspected the product title, on OSX with WebRender enabled.

Maybe a video would be helpful.

The profiler screenshot captures provide some video aspects of the main window. I can try to reproduce on a reference machine and record.

Flags: needinfo?(hkirschner)
Assignee: nobody → mratcliffe
Priority: -- → P3
Assignee: mratcliffe → nobody

It also seems like this line isn't needed anymore: const dbg = this.parent().targetActor.makeDebugger(); because even if we pass dbg later as a second argument to this._eventCollector.hasEventListeners(), the hasEventListeners function actually doesn't need it.

On top of this, yes, it seems like setting this whole event badge mechanism into motion could be done after the inspector has been rendered.
In fact all badges could benefit from this. So kicking off this work could be done after the main update of the inspector was done (which happens when a node is selected). I wonder if wrapping this in a requestIdleCallback is all that's needed. The work still needs to happen for sure (because we want to show the badges), but it doesn't need to block the update.

(In reply to Patrick Brosset <:pbro> from comment #3)

On top of this, yes, it seems like setting this whole event badge mechanism into motion could be done after the inspector has been rendered.
In fact all badges could benefit from this. So kicking off this work could be done after the main update of the inspector was done (which happens when a node is selected). I wonder if wrapping this in a requestIdleCallback is all that's needed. The work still needs to happen for sure (because we want to show the badges), but it doesn't need to block the update.

It would be nice to measure this part to see if how much of a perf win this represents. I would keep in mind of future work such as Bug 1556255.

Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED

(In reply to Gabriel [:gl] (ΦωΦ) from comment #4)

(In reply to Patrick Brosset <:pbro> from comment #3)

On top of this, yes, it seems like setting this whole event badge mechanism into motion could be done after the inspector has been rendered.
In fact all badges could benefit from this. So kicking off this work could be done after the main update of the inspector was done (which happens when a node is selected). I wonder if wrapping this in a requestIdleCallback is all that's needed. The work still needs to happen for sure (because we want to show the badges), but it doesn't need to block the update.

It would be nice to measure this part to see if how much of a perf win this represents. I would keep in mind of future work such as Bug 1556255.

Using requestIdleCallback works well but measuring it is difficult... take this method:

function doit() {
  blah;
  setTimeout(() => { ... }, 10000);
}

As far as the profiler is concerned doit() would take at least 10 seconds to run even though in reality it finished almost instantly. This is because it still needs to display it in the tree.

Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.