Last Comment Bug 685900 - csshtmltree.refreshPanel should use a setTimeout loop to improve performance (cancel-able)
: csshtmltree.refreshPanel should use a setTimeout loop to improve performance ...
Status: RESOLVED FIXED
[styleinspector][minotaur]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 691736
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 09:39 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2011-10-22 12:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
setTimeout loop patch 1 (6.25 KB, patch)
2011-09-20 04:33 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review
setTimeout loop patch 2 (8.03 KB, patch)
2011-10-13 06:13 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
setTimeout loop patch 3 (8.03 KB, patch)
2011-10-13 09:00 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
setTimeout loop patch 4 (8.03 KB, patch)
2011-10-17 02:06 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
setTimeout loop patch 5 (8.45 KB, patch)
2011-10-18 06:25 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Add setTimeout loop 685900 patch 6 (6.25 KB, patch)
2011-10-19 03:27 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-09 09:39:12 PDT
csshtmltree.refreshPanel should use a setTimeout loop to improve performance (cancel-able)
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-20 04:33:52 PDT
Created attachment 561164 [details] [diff] [review]
setTimeout loop patch 1

Done
Comment 2 Mihai Sucan [:msucan] 2011-09-20 10:06:09 PDT
I would prefer a UI fix that doesn't show the number of unmatched rules. That's the part that takes a ton of time to compute in CssLogic. That would speed things up far more.

Rob: what do you think?
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-20 23:04:30 PDT
Yes, but that is part of another bugfix ... Dave Camp asked for both.
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-20 23:05:55 PDT
He asked for this because it slows the breadcrumbs down a lot.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-09-21 05:14:56 PDT
(In reply to Mihai Sucan [:msucan] from comment #2)
> I would prefer a UI fix that doesn't show the number of unmatched rules.
> That's the part that takes a ton of time to compute in CssLogic. That would
> speed things up far more.
> 
> Rob: what do you think?

yeah, I don't know about this. I'd like to know more about why we need it (to speed up breadcrumbs?) and what we could do differently in the Style Inspector to make that not a problem.
Comment 6 Mihai Sucan [:msucan] 2011-09-21 05:23:10 PDT
(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > I would prefer a UI fix that doesn't show the number of unmatched rules.
> > That's the part that takes a ton of time to compute in CssLogic. That would
> > speed things up far more.
> > 
> > Rob: what do you think?
> 
> yeah, I don't know about this. I'd like to know more about why we need it
> (to speed up breadcrumbs?) and what we could do differently in the Style
> Inspector to make that not a problem.

There's work underway that will make these timeouts unneeded:

1. the style inspector UI update will remove the matched/unmatched selector counts, which are a major reason for the sluggishness (especially the unmatched count).

2. two new methods are going to be added to CssLogic: hasMatchedSelectors() and hasUnmatchedSelectors(). These will allow UI to change as needed, with a quick-bailout implementation that doesn't have to do all of the hard work done for retrieving counts or for displaying the list of selectors.

I'm quite positive that with these two fixes (Mike's working on them), we'll be able to remove all timers. If performance will continue to be a problem, then CssHtmlTree remains to be the source of negative perf impact - the UI code.
Comment 7 Mihai Sucan [:msucan] 2011-09-21 08:33:24 PDT
Comment on attachment 561164 [details] [diff] [review]
setTimeout loop patch 1

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

The UI work and the has[un]MatchedSelectors() stuff will help quite more with the perceived tool perf.

My code comment is below.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +247,2 @@
>      }
> +    this._panelRefreshTimeout = this.win.setTimeout(refreshView.bind(this), 0);

I would prefer us to keep this.propertyViews as an object that maps property names to their PropertyView instance. If I am not mistaken, here you can loop through the object using a generator and a for..in loop.
Comment 8 Mihai Sucan [:msucan] 2011-10-07 08:26:53 PDT
Comment on attachment 561164 [details] [diff] [review]
setTimeout loop patch 1

Since we are going to keep this code (for now), I am giving the patch r+.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-13 06:13:29 PDT
Created attachment 566794 [details] [diff] [review]
setTimeout loop patch 2

Fixed failing test
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-13 09:00:52 PDT
Created attachment 566850 [details] [diff] [review]
setTimeout loop patch 3

Rebased
Comment 11 Mihai Sucan [:msucan] 2011-10-14 12:18:38 PDT
Please update dependencies when they change.

Also, please do not write each patch you work on on top of the previous work. This causes us to get into really long patch queues. Please keep dependencies strictly on a technical requirement level. For example this patch does not technically require bug 691736.

(Anyhow, this suggestion should apply only for future patches, not for the existing patch queue. So, please don't apply my suggestion by decoupling the current Style Inspector patch queue :). Thank you!)
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-17 02:06:47 PDT
Created attachment 567403 [details] [diff] [review]
setTimeout loop patch 4

Rebased
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-18 06:25:13 PDT
Created attachment 567728 [details] [diff] [review]
setTimeout loop patch 5

Rebased
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-19 03:27:17 PDT
Created attachment 567996 [details] [diff] [review]
Add setTimeout loop 685900 patch 6

Removed dependencies as part of reordering patch queue
Comment 15 Rob Campbell [:rc] (:robcee) 2011-10-20 14:23:48 PDT
https://hg.mozilla.org/integration/fx-team/rev/a67521de22d5
Comment 16 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-10-22 12:15:11 PDT
https://hg.mozilla.org/mozilla-central/rev/a67521de22d5

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