Last Comment Bug 692543 - Make the Style Inspector UI faster
: Make the Style Inspector UI faster
Status: RESOLVED FIXED
[styleinspector][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Mihai Sucan [:msucan]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 672748
Blocks: 698762
  Show dependency treegraph
 
Reported: 2011-10-06 12:06 PDT by Mihai Sucan [:msucan]
Modified: 2011-11-06 04:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
updated patch (37.96 KB, patch)
2011-10-07 05:02 PDT, Mihai Sucan [:msucan]
dcamp: review+
Details | Diff | Splinter Review
updated patch (37.96 KB, patch)
2011-10-13 06:52 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
updated patch 2 (37.00 KB, patch)
2011-10-13 07:52 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
rebased patch (36.99 KB, patch)
2011-10-17 05:41 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
rebased patch 2 (36.89 KB, patch)
2011-10-26 04:36 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
rebased patch 3 (36.89 KB, patch)
2011-10-27 05:58 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Rebased (36.84 KB, patch)
2011-10-27 06:09 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
rebase and fix (39.92 KB, patch)
2011-10-27 13:28 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Rebase (39.93 KB, patch)
2011-11-02 03:04 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
another rebase (40.37 KB, patch)
2011-11-02 11:09 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Rebase (31.06 KB, patch)
2011-11-03 14:59 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2011-10-06 12:06:46 PDT
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] 2011-10-07 05:02:17 PDT
Created attachment 565497 [details] [diff] [review]
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) 2011-10-10 12:02:36 PDT
Comment on attachment 565497 [details] [diff] [review]
updated patch

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

Looks good!
Comment 3 Mihai Sucan [:msucan] 2011-10-11 03:21:23 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-13 06:52:20 PDT
Created attachment 566802 [details] [diff] [review]
updated patch

Rebased to work on top of the current style inspector patch set
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-13 07:52:37 PDT
Created attachment 566817 [details] [diff] [review]
updated patch 2

Rebased
Comment 6 Mihai Sucan [:msucan] 2011-10-17 05:41:28 PDT
Created attachment 567426 [details] [diff] [review]
rebased patch

Thanks for your patch rebase.

Removed an obsolete comment in the rebase you did. :)
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-26 04:36:36 PDT
Created attachment 569639 [details] [diff] [review]
rebased patch 2
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-27 05:58:47 PDT
Created attachment 569936 [details] [diff] [review]
rebased patch 3
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-27 06:09:53 PDT
Created attachment 569946 [details] [diff] [review]
Rebased
Comment 10 Mihai Sucan [:msucan] 2011-10-27 13:28:01 PDT
Created attachment 570067 [details] [diff] [review]
rebase and fix

Another patch rebase, now with the fix from bug 692400 included.
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-02 03:04:10 PDT
Created attachment 571287 [details] [diff] [review]
Rebase
Comment 12 Mihai Sucan [:msucan] 2011-11-02 11:09:31 PDT
Created attachment 571380 [details] [diff] [review]
another rebase
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-03 14:59:28 PDT
Created attachment 571792 [details] [diff] [review]
Rebase
Comment 14 Mihai Sucan [:msucan] 2011-11-05 04:44:09 PDT
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] 2011-11-05 04:44:34 PDT
Comment on attachment 571380 [details] [diff] [review]
another rebase

This patch still applies cleanly.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-11-05 10:42:54 PDT
https://hg.mozilla.org/integration/fx-team/rev/96f6c274c8f5
Comment 17 Rob Campbell [:rc] (:robcee) 2011-11-06 04:53:46 PST
https://hg.mozilla.org/mozilla-central/rev/96f6c274c8f5

Note You need to log in before you can comment on or make changes to this bug.