692543 - Make the Style Inspector UI faster

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

Building on top of the work being done in bug 685902 and in bug 672748, we need to make the whole Style Inspector UI faster. For this purpose we need changes both in CssLogic and CssHtmlTree.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch to address review comments from bug 685902 comment 11.

I also made changes to build on top of bug 672748.


(In reply to Dave Camp (:dcamp) from bug 685902 comment #11)
> Comment on attachment 563811 [details] [diff] [review] [diff] [details] [review]
> more performance work (wip)
> 
> Review of attachment 563811 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r- mostly for the removed timeouts.  If you think we can guarantee 50ms
> response without those, feel free to renom.
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ -192,5 @@
> >  
> > -      // We use a setTimeout loop to display the properties in batches of 15 at a
> > -      // time. This results in a perceptibly more responsive UI.
> > -      let i = 0;
> > -      let batchSize = 15;
> 
> You removed the timeouts for testing, right?  Do you intend to land this?
> 
> We're generally trying to avoid letting chrome go more than 50ms without
> hitting the event loop (see
> https://wiki.mozilla.org/Firefox/Features/50ms_ASSERT). We need to take that
> into account here.

Good point. No longer removing timeouts, just making *some* adjustments. Personally I am "70% happy" with the performance on Ubuntu.com and Github.

(even removing the timers wouldn't be too bad)

With this patch things do feel much faster on my system.


> ::: browser/devtools/styleinspector/CssLogic.jsm
> @@ +588,5 @@
> >     * the domRules for the element are passed to the callback function.
> >     *
> >     * @param {function} [aCallback] Simple callback method
> >     */
> > +  hasMatchedSelectors: function CL_hasMatchedSelectors(aProperties)
> 
> This method no longer matches its description.

Fixed. It was only a WIP. :)


> @@ +605,5 @@
> > +    if (this._matchedRules) {
> > +      if (aProperties.length > 0) {
> > +        this._matchedRules.some(function(aRule) {
> > +          aProperties = aProperties.filter(propertiesFilter.bind(this, aRule));
> > +          return aProperties.length == 0;
> 
> The logic for building up |result| is just complicated enough that
> duplicating it (one way when _matchedRules already exists, another when
> building _matchedRules) that it took some time to verify both paths.  Might
> be nicer to just have:
> 
> if (!this._matchedRules) {
>   // regen matched rules, maybe split up into its own method
> }
> 
> this._matchedRules.some(aProperties.filter(// etc
> 
> Splitting out the matched-rule-regeneration into its own method would make
> all the various callsites that have bare hasMatchedSelectors() calls less
> weird too.

Good points. Fixed!


> @@ +660,3 @@
> >        }
> > +    } while ((element = element.parentNode) &&
> > +             element.nodeType === Ci.nsIDOMNode.ELEMENT_NODE);
> 
> Err, this parent node stuff seems incorrect.  It should take into account
> the fact that some properties (like text alignment) are inherited from
> parent nodes, but others (like borders) aren't.
> 
> This patch didn't introduce this problem, but ick.  I filed bug 691978.

True. Thanks for the report!


> @@ +669,5 @@
> >     * match the highlighted element or its parent elements.
> >     *
> >     * @param {String} aProperty The CSS property to check against
> >     */
> > +  hasUnmatchedSelectors: function CL_hasUnmatchedSelectors(aProperties)
> 
> This comment needs updating too.
> 
> Since on the surface they have such similar APIs, it's probably worth
> mentioning in the comments that hasUnmatchedSelectors() and
> hasMatchedSelectors() have vastly different performance characteristics.

Fixed.

> @@ +718,5 @@
> > +          result[aProperty] = unmatched;
> > +
> > +          // Keep this property if the rule matched. We need to find if a rule
> > +          // has this property while it doesn't match the viewedElement.
> > +          return !unmatched;
> 
> If unmatched was false, we would have bailed out above the filter() call,
> right?  Will this ever return true?

Hah, good catch! I wrote the filter function before I wrote the logic that skips rules entirely based on their matched/unmatched state, and I forgot to update the filter accordingly. Thanks for pointing this out.


Thank you Dave for your review! Looking forward for more comments!

Comment 2

[Dave Camp (:dcamp)]

Comment on attachment 565497 [details] [diff] [review]
updated patch

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

Looks good!

Comment 3

[Mihai Sucan [:msucan]]

Thank you Dave for the r+!

A note on the expected performance with this patch: first open is slowest, it takes up to a second on github.org and ubuntu.com. Subsequent highlights of elements, expanding of matched selectors and so on... are (close to) instant.

There are edge cases where subsequent highlights of elements, or expansions of matched selectors that take up to a second - similar to how long a first open takes. These "edge cases" depend on which element is highlighted, which rules match, which don't, and how the styles of the page are structured. Some cause more code to run.

Switching between showing UA styles or not is also (close to) instant (in all cases).

Comment 4

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

Attachment: updated patch

Rebased to work on top of the current style inspector patch set

Comment 5

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

Attachment: updated patch 2

Rebased

Comment 6

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Thanks for your patch rebase.

Removed an obsolete comment in the rebase you did. :)

Comment 7

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

Attachment: rebased patch 2

Comment 8

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

Attachment: rebased patch 3

Comment 9

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

Attachment: Rebased

Comment 10

[Mihai Sucan [:msucan]]

Attachment: rebase and fix

Another patch rebase, now with the fix from bug 692400 included.

Comment 11

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

Attachment: Rebase

Comment 12

[Mihai Sucan [:msucan]]

Attachment: another rebase

Comment 13

[Rob Campbell [:rc] (:robcee)]

Attachment: Rebase

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 571792 [details] [diff] [review]
Rebase

Rob: this patch is missing the CssLogic.shortSource() fix I have in attachment 571380 [details] [diff] [review].

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 571380 [details] [diff] [review]
another rebase

This patch still applies cleanly.

Comment 16

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/96f6c274c8f5

Comment 17

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/96f6c274c8f5