Closed Bug 976527 Opened 10 years ago Closed 9 years ago

Inspector slows down isbitcoindeadyet.com *a lot*

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: paul, Unassigned)

References

Details

Attachments

(2 files)

STR:
- open http://isbitcoindeadyet.com/
- page is smooth
- open the toolbox with the webconsole
- page is smooth
- open the inspector
- page is slow
The reason is because the site is applying transform properties constantly using JS to animate 20+ coins on the page, thus updating the node's properties, which in turn flashes and updates the markupview constantly. Making the whole process slow.

IMO, the site should ideally use animations to do the same. Then the issue will not be there.
With local debugging: both the site and the devtools are slow.
With remote debugging (via the Connect screen): the site is fine but the devtools are slow.
(In reply to Girish Sharma [:Optimizer] from comment #1)
> IMO, the site should ideally use animations to do the same. Then the issue
> will not be there.

I don't think the devtools should only behave correctly on correctly coded websites :)

We should not slow down the page no matter what.
With E10S coming down the pipe anyway, we'll have to make devtools run in a separate process, so at least, our devtools should never slow down sites anymore after we've migrated.

Nevertheless, the inspector is really really slow on that page. We should probably throttle the updates made to the markup-view on markup mutations.
The only thing that can be done here to speed up things is to batch the packets for showing and flashing updates for the markup view (like I am doing for Storage Actor updates).

But otherwise, this slowness is by design that we should show the changes and highlight the node live when any change in DOM happens.
Ok, so disabling completely the rule-view and computed-view doesn't seem to make any difference.
However, disabling the markup-mutation observer in the markup-view makes both the site and the devtools responsive again.

Several things can be tested from there on:
- batch mutation packets
- throttle the markup-view refresh rate
- look at our MarkupContainer update logic, see if it can be made faster
After more investigation, it seems like what uses the most of the processing time is the templating part in the markup-view.
Of course there are other things too (one candidate would also be the breadcrumb which refreshes at a very high rate too, for nothing), but the template part is the first part to be focused on.
https://dl.dropboxusercontent.com/u/714210/mozilla/bitcoin-site-inspector-profile.json
This is a profile recorded with the browser toolbox's profiler.
You can import it in your profiler.

What this tells us is that in Templater.jsm:

/**
 * Recursive function to walk the tree processing the attributes as it goes.
 * @param node the node to process. If you pass a string in instead of a DOM
 * element, it is assumed to be an id for use with document.getElementById()
 * @param data the data to use for node processing.
 */
Templater.prototype.processNode = function(node, data) {...}

is the function that takes the most time to run when reacting to markup-mutations.

In markup-view, ElementEditor.prototype.update is called when updating the node, and it does:

// Get the attribute editor for each attribute that exists on
// the node and show it.
for (let attr of attrs) {
  let attribute = this._createAttribute(attr);
  if (!attribute.inplaceEditor) {
    attribute.style.removeProperty("display");
  }
}

Which then calls:

this.template("attribute", data);
Attached file platform data enabled
"Profiling with platform data enabled"

Shows that 15% of the time is spent on painting. This includes the flashing of 10+ rows.

Screenshot : http://i.snag.gy/6Fs9n.jpg
On stepping through calls and stuff, I found that when an attribute change happens, in a markup mutation, we hide *all* the old attributes (i.e. the nodes representing the attributes) in the following lines :

ElementEditor.prototype = {
  /**
   * Update the state of the editor from the node.
   */
  update: function() {
    let attrs = this.node.attributes;
    if (!attrs) {
      return;
    }

    // Hide all the attribute editors, they'll be re-shown if they're
    // still applicable.  Don't update attributes that are being
    // actively edited.
    let attrEditors = this.attrList.querySelectorAll(".attreditor");
    for (let i = 0; i < attrEditors.length; i++) {
      if (!attrEditors[i].inplaceEditor) {
        attrEditors[i].style.display = "none";
      }
    }

Then we show them again (only which are applicable again)


    // Get the attribute editor for each attribute that exists on
    // the node and show it.
    for (let attr of attrs) {
      let attribute = this._createAttribute(attr);
      if (!attribute.inplaceEditor) {
        attribute.style.removeProperty("display");
      }
    }

I am assuming that this will cause a lot of reflows and recalculations and thus makes the whole process slow.

It would be much faster to only remove and add removed and added attributes and just update the textContent of the changed attributes' values.

PS: i removed the flash, stopped the breadcrumbs update on attribute change and disabled the sidebar. No visible perf improvement as suggested by Patrick.
(In reply to Girish Sharma [:Optimizer] from comment #10)

Nope. I added "display: none" style on each of the row of markup view . Still the same slowness.
So, after more testing, the templating mechanism, is only half the problem on this site.
As Optimizer said, we are hiding all attributes at each update, creating new ones (even if they have the same name and value) and showing them.
Just by adding a simple test at the beginning of the function, I was able to improve the situation on this site quite dramatically.
The templating still seems to be part of the problem as removing the call to template(...) and instead creating nodes with doc.creatElement and such seemed to make the page even faster.

Let's keep in mind that this site is only one usecase, we should also look into pages that append and remove nodes in a rapid fashion. In this case, the _createAttributes function isn't going to be much of a problem.

Last, I also changed a little bit the way the flashMutation function works as for now, it changes classNames everytime, even if not needed, and it seemed to have had some impacts on the performance.

So, anyway, there are obvious performance improvements we can make on the markup-view, and I think there's enough in the comments of this bug to get started. I don't have time at the moment to work on it, so I'm leaving this unassigned.
Attached image slow-inspector.gif
This is another example of the inspector being slow on certain sites.
This one is dhtmlconf.com
Just opening the inspector makes the site really slow, and that's mostly coming from the animated particles that follow the mouse around on the site.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> So, after more testing, the templating mechanism, is only half the problem
> on this site.
> As Optimizer said, we are hiding all attributes at each update, creating new
> ones (even if they have the same name and value) and showing them.
> Just by adding a simple test at the beginning of the function, I was able to
> improve the situation on this site quite dramatically.
> The templating still seems to be part of the problem as removing the call to
> template(...) and instead creating nodes with doc.creatElement and such
> seemed to make the page even faster.
> 
> Let's keep in mind that this site is only one usecase, we should also look
> into pages that append and remove nodes in a rapid fashion. In this case,
> the _createAttributes function isn't going to be much of a problem.
> 
> Last, I also changed a little bit the way the flashMutation function works
> as for now, it changes classNames everytime, even if not needed, and it
> seemed to have had some impacts on the performance.
> 
> So, anyway, there are obvious performance improvements we can make on the
> markup-view, and I think there's enough in the comments of this bug to get
> started. I don't have time at the moment to work on it, so I'm leaving this
> unassigned.

Do you have a WIP patch with these changes?  It'd be worth landing some of the simple performance fixes, even if it doesn't fix all use cases.
Also FYI, the profiling data you took earlier could have been unreliable since this was before we had the capability to *not* load the debugger panel first.  I would often see totally different results when profiling the same thing multiple times.  Now if you open the browser toolbox, then switch the the profiler tab, then restart it will open on profiler by default next time and should hopefully be more reliable.
ni? for comment 14
Flags: needinfo?(pbrosset)
No sorry, these were just quick tests I had made, I didn't save any of it in a patch.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> With E10S coming down the pipe anyway, we'll have to make devtools run in a
> separate process, so at least, our devtools should never slow down sites
> anymore after we've migrated.
> 
> Nevertheless, the inspector is really really slow on that page. We should
> probably throttle the updates made to the markup-view on markup mutations.

Bug 1080884 was logged about this issue, which somehow becomes *worse* on E10S builds.
See Also: → 1080884
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> > With E10S coming down the pipe anyway, we'll have to make devtools run in a
> > separate process, so at least, our devtools should never slow down sites
> > anymore after we've migrated.
> > 
> > Nevertheless, the inspector is really really slow on that page. We should
> > probably throttle the updates made to the markup-view on markup mutations.
> 
> Bug 1080884 was logged about this issue, which somehow becomes *worse* on
> E10S builds.

Unsurprising since the overhead of each RDP request is likely greater in e10s (I haven't actually measured, but it sure makes sense that inter-process communication is more expensive than intra-process communication).
The original example for this bug has an expired / parked domain name now.
Brian made a couple of important changes recently focusing specifically on the markup-view update performance.
The site is closed now so it's not possible to test anymore, but we've done similar tests with js animations and there's now no apparent lag anymore (http://bgrins.github.io/devtools-demos/inspector/jquery-animation.html).
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: