The default bug view has changed. See this FAQ.

Make the Style Inspector UI faster

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleinspector][fixed-in-fx-team])

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #565497 - Flags: review?(dcamp)

Comment 2

6 years ago
Comment on attachment 565497 [details] [diff] [review]
updated patch

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

Looks good!
Attachment #565497 - Flags: review?(dcamp) → review+
(Assignee)

Comment 3

6 years ago
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).
Created attachment 566802 [details] [diff] [review]
updated patch

Rebased to work on top of the current style inspector patch set
Attachment #565497 - Attachment is obsolete: true
Created attachment 566817 [details] [diff] [review]
updated patch 2

Rebased
Attachment #566802 - Attachment is obsolete: true
Blocks: 692400
(Assignee)

Comment 6

6 years ago
Created attachment 567426 [details] [diff] [review]
rebased patch

Thanks for your patch rebase.

Removed an obsolete comment in the rebase you did. :)
Attachment #566817 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
No longer blocks: 692400
Created attachment 569639 [details] [diff] [review]
rebased patch 2
Attachment #567426 - Attachment is obsolete: true
Whiteboard: [styleinspector] → [styleinspector][r+]
Created attachment 569936 [details] [diff] [review]
rebased patch 3
Attachment #569639 - Attachment is obsolete: true
Created attachment 569946 [details] [diff] [review]
Rebased
Attachment #569936 - Attachment is obsolete: true

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 10

6 years ago
Created attachment 570067 [details] [diff] [review]
rebase and fix

Another patch rebase, now with the fix from bug 692400 included.
Attachment #569946 - Attachment is obsolete: true
Created attachment 571287 [details] [diff] [review]
Rebase
Attachment #570067 - Attachment is obsolete: true
Blocks: 698762
(Assignee)

Comment 12

6 years ago
Created attachment 571380 [details] [diff] [review]
another rebase
Attachment #571287 - Attachment is obsolete: true
Created attachment 571792 [details] [diff] [review]
Rebase
Attachment #571380 - Attachment is obsolete: true
Whiteboard: [styleinspector][r+] → [styleinspector][land-in-fx-team]
(Assignee)

Comment 14

6 years ago
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].
Attachment #571792 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Comment on attachment 571380 [details] [diff] [review]
another rebase

This patch still applies cleanly.
Attachment #571380 - Attachment is obsolete: false
https://hg.mozilla.org/integration/fx-team/rev/96f6c274c8f5
Whiteboard: [styleinspector][land-in-fx-team] → [styleinspector][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/96f6c274c8f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.