931191 - Inspector janks for seconds at a time looking at chat.meatspac.es

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Jim Blandy :jimb]

Visit chat.meatspac.es, open the DOM inspector, click on the follow-the-mouse node highlighting, and then mouse over the chat entries. The inspector hangs for seconds at a time.

This may be related to the fact that all the animated images are animated GIFs specified by gigantic data:image/gif;base64 URLs.

Comment 1

[Dave Camp (:dcamp)]

We truncate attributes for display, but I don't think we truncate them over the wire.

Comment 2

[Dave Camp (:dcamp)]

Attachment: v1

This has three components:

a) Only send partial attributes over the wire, and ask for the full version at edit time.
b) A 'loading' state for the inplace editor, used while asking for the full version.
c) Drastically increase the read length for reading strings, because there was way too much latency getting in the way there.

Comment 3

[Dave Camp (:dcamp)]

https://tbpl.mozilla.org/?tree=Try&rev=3f1bfaf1f1e2 (I bet something will fail, I wasn't too thorough on local testing).

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 824833 [details] [diff] [review]
v1

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

It speeds up chat.meatspac.es as promised. 

A few notes about the UI:
1) Since the value being sent to the frontend is a max of 50 characters, it is no longer truncated on the frontend.  The COLLAPSE_ATTRIBUTE_LENGTH and COLLAPSE_DATA_URL_LENGTH values should be set to gValueSummaryLength - 1 to reenable the truncation on the frontend.
2) It seems like I've been able to get the editor to stick open sometimes on the meatspac.es site, meaning that it loses focus and won't go away when clicking on another node, pressing escape, etc.  The only way to clear it is to first refocus the input.  I suspect it has to do with the editor loading during a mutation or something, but haven't been able to consistently reproduce (just continuously opening / closing the editor for the src attribute on a gif and it happens sometimes).  Maybe we need to make sure that the promise is being cleaned up in the inplace editor destroy functionality or something?  I will see if I can come up with a more concrete STR.
3) If you inspect a gif near the bottom of the page and double click to open the attribute editor, the markup tree scrolls almost out view and the whole scrollable region becomes huge vertically.  This does seem to be a bug even without the patch applied (albeit without the automatic scrolling) so we can probably figure that out separately.

::: browser/devtools/markupview/markup-view.js
@@ +1663,5 @@
>      };
>      this.template("attribute", data);
>      var {attr, inner, name, val} = data;
>  
> +    let self = this;

`self` isn't used here, can be removed

::: browser/devtools/markupview/test/browser_bug896181_css_mixed_completion_new_attribute.js
@@ +12,5 @@
>  
>  function test() {
>    let inspector;
>    let {
> +    getLoadedInplaceEditorForSpan: inplaceEditor

please import variable this as `loadedInplaceEditor` for both tests, to help distinguish between the `inplaceEditor` function used in other tests.

::: toolkit/devtools/server/actors/inspector.js
@@ +226,5 @@
> +        value: attr.value.substring(0, gValueSummaryLength),
> +        incomplete: true
> +      }
> +    }
> +    return attr.substring(0, 50);

Extra return statement that never gets executed

Comment 5

[Dave Camp (:dcamp)]

(In reply to Brian Grinstead [:bgrins] from comment #4)

> 1) Since the value being sent to the frontend is a max of 50 characters, it
> is no longer truncated on the frontend.  The COLLAPSE_ATTRIBUTE_LENGTH and
> COLLAPSE_DATA_URL_LENGTH values should be set to gValueSummaryLength - 1 to
> reenable the truncation on the frontend.

Assuming 120 chars was chosen for a reason, should we truncate attrs at 120 instead of 50 in the protocol?

> 3) If you inspect a gif near the bottom of the page and double click to open
> the attribute editor, the markup tree scrolls almost out view and the whole
> scrollable region becomes huge vertically.  This does seem to be a bug even
> without the patch applied (albeit without the automatic scrolling) so we can
> probably figure that out separately.

Yeah, let's split that out.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Dave Camp (:dcamp) from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> 
> > 1) Since the value being sent to the frontend is a max of 50 characters, it
> > is no longer truncated on the frontend.  The COLLAPSE_ATTRIBUTE_LENGTH and
> > COLLAPSE_DATA_URL_LENGTH values should be set to gValueSummaryLength - 1 to
> > reenable the truncation on the frontend.
> 
> Assuming 120 chars was chosen for a reason, should we truncate attrs at 120
> instead of 50 in the protocol?
> 

The number was somewhat arbitrarily chosen as what was a couple of lines of wrapping.  But yes, if that won't negatively affect performance in other cases, it would be good to keep it at what we had decided earlier (this will also allow data URLs to get collapsed earlier than normal attribute values as originally designed).  Although I believe we will have to either set this one at 121 or set COLLAPSE_ATTRIBUTE_LENGTH at 119 in order to trigger the actual ellipses.

Comment 7

[Dave Camp (:dcamp)]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Dave Camp (:dcamp) from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > 
> > > 1) Since the value being sent to the frontend is a max of 50 characters, it
> > > is no longer truncated on the frontend.  The COLLAPSE_ATTRIBUTE_LENGTH and
> > > COLLAPSE_DATA_URL_LENGTH values should be set to gValueSummaryLength - 1 to
> > > reenable the truncation on the frontend.
> > 
> > Assuming 120 chars was chosen for a reason, should we truncate attrs at 120
> > instead of 50 in the protocol?
> > 
> 
> The number was somewhat arbitrarily chosen as what was a couple of lines of
> wrapping.  But yes, if that won't negatively affect performance in other
> cases, it would be good to keep it at what we had decided earlier (this will
> also allow data URLs to get collapsed earlier than normal attribute values
> as originally designed).  Although I believe we will have to either set this
> one at 121 or set COLLAPSE_ATTRIBUTE_LENGTH at 119 in order to trigger the
> actual ellipses.

We could rework the ellipses to be based on whether the protocol tells us it had to truncate or not.

Comment 8

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > (In reply to Dave Camp (:dcamp) from comment #5)
> > > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > 
> > > > 1) Since the value being sent to the frontend is a max of 50 characters, it
> > > > is no longer truncated on the frontend.  The COLLAPSE_ATTRIBUTE_LENGTH and
> > > > COLLAPSE_DATA_URL_LENGTH values should be set to gValueSummaryLength - 1 to
> > > > reenable the truncation on the frontend.
> > > 
> > > Assuming 120 chars was chosen for a reason, should we truncate attrs at 120
> > > instead of 50 in the protocol?
> > > 
> > 
> > The number was somewhat arbitrarily chosen as what was a couple of lines of
> > wrapping.  But yes, if that won't negatively affect performance in other
> > cases, it would be good to keep it at what we had decided earlier (this will
> > also allow data URLs to get collapsed earlier than normal attribute values
> > as originally designed).  Although I believe we will have to either set this
> > one at 121 or set COLLAPSE_ATTRIBUTE_LENGTH at 119 in order to trigger the
> > actual ellipses.
> 
> We could rework the ellipses to be based on whether the protocol tells us it
> had to truncate or not.

Never mind, that doesn't seem like it'll work.  I'll set COLLAPSE_ATTRIBUTE_LENGTH to 119 and send 120 characters.

Comment 9

[Dave Camp (:dcamp)]

Of course, that'll leave us ellipsing a truncated-at-the-end value in the middle.

Comment 10

[Dave Camp (:dcamp)]

OK, so the addition of the output parser and truncation makes this more complicated than I hoped it would be.

We can do some truncation on the server side, but without doing the entire output parser on the server side, it's not going to be as good as it is now.  I don't feel great doing the entire output parser on the server, but maybe that's the way to go.

Thoughts?

Comment 11

[Dave Camp (:dcamp)]

hrm.  Local transport, where this is breaking, shouldn't be adding significant transport overhead.  So this might not even be the right problem to be solving.

Maybe the output parser is slow?

Comment 12

[Dave Camp (:dcamp)]

This needs more attention than I can give it right now.  Maybe bgrins or mratcliffe can profile and see what's taking too much time?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Dave Camp (:dcamp) from comment #11)
> hrm.  Local transport, where this is breaking, shouldn't be adding
> significant transport overhead.  So this might not even be the right problem
> to be solving.
> 
> Maybe the output parser is slow?

This patch definitely seems to be addressing the performance issue.  Are you thinking this is just because you increased the LONG_STRING_READ_LENGTH?

Comment 14

[Dave Camp (:dcamp)]

Attachment: bypass-output-parser.diff

But this fixes the performance problem too, even without any protocol changes.

Comment 15

[Dave Camp (:dcamp)]

(I do think we should increase LONG_STRING_READ_LENGTH though)

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Dave Camp (:dcamp) from comment #14)
> Created attachment 827082 [details] [diff] [review]
> bypass-output-parser.diff
> 
> But this fixes the performance problem too, even without any protocol
> changes.

Mike, looks like the output parser is causing a slowdown on very long attributes.  Mind taking a look at it?  You can see it by inspecting one of the gifs on chat.meatspac.es with and without Attachment 827082 [details] [diff] applied.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I am surprised that there is such a slowdown but I can see why. The current output parser didn't parse URLs so it will parse the URL text in chunks, which would cause a significant slowdown if you have a lot of long data URLs.

The parser was simplified and to a large extent rewritten for bug 929384 but I need to address Patrick's comments before it lands.

This bug really needs to wait on that change, setting as dependent on bug 929384.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: speedup-output-parser.patch

Mike,
Can you have a look at this patch?  It does two things to the output parser:

1) It sets a MAX_ITERATIONS constant which upon being reached, will dump the rest of the text into a text node and end the loop.
2) It changes the intermediate storage in this.parsed for text nodes in to be strings instead, which also seems to speed things up (I think in this particular case the `lastItem.nodeValue += text` was getting slow because there were so many iterations (around 26K on one gif I tested).

I realize that this particular case will also be handled by the URL matching in Bug 929384, but we should prevent this kind of slowdown from any input (even if it isn't a data URL).

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=68f7535f051e

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 827464 [details] [diff] [review]
speedup-output-parser.patch

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

Very simple idea to use a string instead of a text node ... I like it.

100 iterations seems a little high but I guess we should leave it there for the moment.

Comment 20

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/1238b4fee5b5
https://tbpl.mozilla.org/?tree=Fx-Team&rev=1238b4fee5b5

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: longer-long-string.patch

Think this is still worth doing?

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1238b4fee5b5

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 828291 [details] [diff] [review]
longer-long-string.patch

https://hg.mozilla.org/integration/fx-team/rev/77c68153184c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77c68153184c

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 827464 [details] [diff] [review]
speedup-output-parser.patch

We need to uplift this in order to land bug 939098, which is already approved.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DevTools output parser
User impact if declined: Class names in markup panel may be replaced by color swatches e.g. class="red" would be displayed as class="#F00", class="inherit" would be displayed as class="black", CSS shortcut properties such as border: 1px solid red will not receive color swatches plus various other issues.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff70754e42bc

Calling this "fixed" on Fx27 for tracking purposes, but I recognize that only one patch from this bug landed on it.

Comment 27

[Ioana (away)]

Verified as fixed on Ubuntu 12.10 x86_64 using the 01/16 Firefox 28.

It also works a lot better on Firefox 27 (verified on beta 7), though it's somewhat slower than 28.