Trevor Rowbotham [:rowbot] Reporter
	Description • 10 years ago

STR:
1) Load the URL with e10s enabled.
2) Open the Inspector
3) Expand the <div> tag that contains all the moving circles.

Actual Results
The responsiveness of the browser becomes very slow and after about 30 seconds, attempting to interact with the browser or dev tools causes the browser to hang or become unresponsive.  Also, the styles don't really get updated.

Expected results
For the browser to not hang or become unresponsive and for style updates to be reflected in the dev tools.

Inspecting this page with e10s disabled just causes the browser to become painfully slow, but it doesn't hang or become unresponsive.

	Dave Camp (:dcamp)
	Comment 1 • 10 years ago

Not having actually profiled yet, I bet this is congestion of mutation events, and we could improve this with some work to batch those up.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 10 years ago

Slightly simplified local test

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 3 • 10 years ago

As expected, the templater points us towards Templater.jsm... we should see what we can do there in order to speed things up.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 4 • 10 years ago

Update is calling _createAttribute for every attribute regardless of whether a current editor exists or not.

By reusing attribute editors when they already exist the period of time it takes update() and it's ancestral functions to run decreases from 2166.17ms to 13.8ms.

Of course, if an attribute doesn't already exist we need to use the slower code path but a 15600% increase in this particular code path really helps.

The problem now is that because the editor is reused it contains the value for the previous attribute. The editor itself is not accessible from the inplace editor so we need a way to reset the initial value.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 5 • 10 years ago

We now recycle markup view editors when possible. This results a very large speed increase when lots of elements are updated at the same time.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=687f76b7e681
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=687f76b7e681

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 6 • 10 years ago

The try is orange but the errors are not to do with this patch... they are caused by the highlighter patch.

	Brian Grinstead [:bgrins]
	Comment 7 • 10 years ago

Comment on attachment 8508591 [details] [diff] [review]
speed-up-markup-view-1080884.patch

Review of attachment 8508591 [details] [diff] [review]:
-----------------------------------------------------------------

I have some questions about the general approach here before further reviewing.  It seems like there should be a simpler way to void out / destroy the inplace editor after a change to the editableField rather than detecting via textContent and marking a flag on the DOM node that gets read in the inplaceEditor constructor.

::: browser/devtools/markupview/markup-view.js
@@ +2065,5 @@
>      // the node and show it.
>      for (let attr of attrs) {
> +      let attrEditor = this.attrs[attr.name];
> +      if (attrEditor) {
> +        let collapsedValue;

There must be a better way to check if the attribute editor already exists (and hasn't been changed).  If we do need this here, the collapsing logic should be extracted into a function and called from both places so we don't accidentally change it only in one place

@@ +2076,5 @@
> +        // The attribute editor already exisis so we reuse it.
> +        if (attrEditor.querySelector(".attr-value").textContent !== collapsedValue) {
> +          attrEditor.querySelector(".attr-value").textContent = collapsedValue;
> +
> +          // The node has no access to the inline editor, which is linked to

Could we instead just rebuild the attribute with _createAttribute if it's changed?  This isn't going to happen very often (only when the user has made a change), and it seems more straightforward.

::: browser/devtools/shared/inplace-editor.js
@@ +178,5 @@
> +
> +  // If the node has a resetEditor flag we need to ignore aOptions.initial as it
> +  // no longer applies due to the editor being recycled.
> +  if (this.elt.resetEditor) {
> +    this.initial = this.elt.textContent;

The point in our case of passing in an initial was that we had a chance to escape quotes and generally clean up the editor (see _createAttribute).  Seems like skipping that could open up the editor for bugs.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 8 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8508591 [details] [diff] [review]
> speed-up-markup-view-1080884.patch
> 
> Review of attachment 8508591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions about the general approach here before further
> reviewing.  It seems like there should be a simpler way to void out /
> destroy the inplace editor after a change to the editableField rather than
> detecting via textContent and marking a flag on the DOM node that gets read
> in the inplaceEditor constructor.
> 
> ::: browser/devtools/markupview/markup-view.js
> @@ +2065,5 @@
> >      // the node and show it.
> >      for (let attr of attrs) {
> > +      let attrEditor = this.attrs[attr.name];
> > +      if (attrEditor) {
> > +        let collapsedValue;
> 
> There must be a better way to check if the attribute editor already exists
> (and hasn't been changed).  If we do need this here, the collapsing logic
> should be extracted into a function and called from both places so we don't
> accidentally change it only in one place
> 

The big problem here is that the node has no information about the attached editor. It's only link is the e.g. dblclick event... everything else is stored in the editor.

Maybe we could add a refreshEditor method to the node when an editor is attached, that certainly seems much tidier and that way we could pass in a new initial value.

	Brian Grinstead [:bgrins]
	Comment 9 • 10 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > Comment on attachment 8508591 [details] [diff] [review]
> > speed-up-markup-view-1080884.patch
> > 
> > Review of attachment 8508591 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I have some questions about the general approach here before further
> > reviewing.  It seems like there should be a simpler way to void out /
> > destroy the inplace editor after a change to the editableField rather than
> > detecting via textContent and marking a flag on the DOM node that gets read
> > in the inplaceEditor constructor.
> > 
> > ::: browser/devtools/markupview/markup-view.js
> > @@ +2065,5 @@
> > >      // the node and show it.
> > >      for (let attr of attrs) {
> > > +      let attrEditor = this.attrs[attr.name];
> > > +      if (attrEditor) {
> > > +        let collapsedValue;
> > 
> > There must be a better way to check if the attribute editor already exists
> > (and hasn't been changed).  If we do need this here, the collapsing logic
> > should be extracted into a function and called from both places so we don't
> > accidentally change it only in one place
> > 
> 
> The big problem here is that the node has no information about the attached
> editor. It's only link is the e.g. dblclick event... everything else is
> stored in the editor.

When you say editor here are you talking about an editableItem, editableField, or InplaceEditor?

> Maybe we could add a refreshEditor method to the node when an editor is
> attached, that certainly seems much tidier and that way we could pass in a
> new initial value.

Yeah, something that removes the level of indirection would be good.  I also just had a thought that since the aOptions object passed around within inplaceeditor.js is by reference we could just store the options object in a variable within markup-view.js [0] and set opts.initial = "foo" inside of the done callback.  Would that have the same effect?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 10 • 10 years ago

T(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> > (In reply to Brian Grinstead [:bgrins] from comment #7)
> > > Comment on attachment 8508591 [details] [diff] [review]
> > > speed-up-markup-view-1080884.patch
> > > 
> > > Review of attachment 8508591 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I have some questions about the general approach here before further
> > > reviewing.  It seems like there should be a simpler way to void out /
> > > destroy the inplace editor after a change to the editableField rather than
> > > detecting via textContent and marking a flag on the DOM node that gets read
> > > in the inplaceEditor constructor.
> > > 
> > > ::: browser/devtools/markupview/markup-view.js
> > > @@ +2065,5 @@
> > > >      // the node and show it.
> > > >      for (let attr of attrs) {
> > > > +      let attrEditor = this.attrs[attr.name];
> > > > +      if (attrEditor) {
> > > > +        let collapsedValue;
> > > 
> > > There must be a better way to check if the attribute editor already exists
> > > (and hasn't been changed).  If we do need this here, the collapsing logic
> > > should be extracted into a function and called from both places so we don't
> > > accidentally change it only in one place
> > > 
> > 
> > The big problem here is that the node has no information about the attached
> > editor. It's only link is the e.g. dblclick event... everything else is
> > stored in the editor.
> 
> When you say editor here are you talking about an editableItem,
> editableField, or InplaceEditor?
> 
> > Maybe we could add a refreshEditor method to the node when an editor is
> > attached, that certainly seems much tidier and that way we could pass in a
> > new initial value.
> 
> Yeah, something that removes the level of indirection would be good.  I also
> just had a thought that since the aOptions object passed around within
> inplaceeditor.js is by reference we could just store the options object in a
> variable within markup-view.js [0] and set opts.initial = "foo" inside of
> the done callback.  Would that have the same effect?

I meant the Inplace Editor but the options object is totally what we need to target. I had decided on using a synthetic event for communication but using a WeakMap of option objects sounds like a far better approach, and much simpler.

/me gets onto it.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 11 • 10 years ago

Reusing the editor comes with it's own set of problems:

1. Creating a new attribute using a space and adding a new attribute results in an editor encompassing both attributes. Double clicking one of the attributes recreates an editor with just the attribute values. If the editor is reused it is not tied to a single attribute.
2. Keeping track of which attribute was last edited and all of the initial values becomes a nightmare.

I suspect that reimplementing our inplace editor itself would make a lot of sense considering that we already have the initial text and it only needs a little manipulation.

Moving on to other issues as I have already spent too much time on this.

	Chris Peterson [:cpeterson]
	Comment 12 • 10 years ago

These DevTools bugs should probably block e10s from riding to Aurora.

	Chris Peterson [:cpeterson]
	Comment 13 • 10 years ago

devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 14 • 10 years ago

Also see bug 976527

	Eddy Bruel [:ejpbruel]
	Comment 15 • 9 years ago

Mike says that we're slow at updating lots of nodes in general, not just with E10s. It is just a little worse on E10s (about half the speed).

The problem is that we recreate every node on every attribute change. To fix this issue would require a rewrite of the markup view. This would be a substantial amount of work. However, to put the issue in perspective, to trigger it you need to have thousands of nodes visible in the markup view and change attributes on them all continually.

N.B. Ironically, React with its DOM diffing would be a perfect solution to this.

	Trevor Rowbotham [:rowbot] Reporter
	Comment 16 • 9 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #15)
> Mike says that we're slow at updating lots of nodes in general, not just
> with E10s. It is just a little worse on E10s (about half the speed).
> 
> The problem is that we recreate every node on every attribute change. To fix
> this issue would require a rewrite of the markup view. This would be a
> substantial amount of work. However, to put the issue in perspective, to
> trigger it you need to have thousands of nodes visible in the markup view
> and change attributes on them all continually.
> 
> N.B. Ironically, React with its DOM diffing would be a perfect solution to
> this.

I'm sorry to hear that this will require a lot of work to fix.  I realize that the example in the URL I originally provided was pretty overkill with some 700+ nodes but, I am able to trigger this with a mere 20 nodes with constantly changing attributes with e10s enabled.  With only 20 nodes, the browser comes in and out of a unresponsive state every 15-20 seconds, and styles still fail to update.  So, while the symptoms are not as bad as they are with 1000 nodes, the dev tools are still very much unusable with 20 nodes.

	Trevor Rowbotham [:rowbot] Reporter
	Comment 17 • 9 years ago

Bug 1139569 significantly improved the situation here as pointed out in Bug 1139569 comment #5.  It is still pretty slow on the testcase in this bug, but it is usable now and no longer hangs the browser.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 18 • 9 years ago

(In reply to Trevor Rowbotham from comment #17)
> Bug 1139569 significantly improved the situation here as pointed out in Bug
> 1139569 comment #5.  It is still pretty slow on the testcase in this bug,
> but it is usable now and no longer hangs the browser.

Thanks for your feedback.

Filter on 1ff0543e-b501-4893-a72b-e4773c01e655