Last Comment Bug 704911 - Unusable performance selecting text on github
: Unusable performance selecting text on github
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
https://github.com/TTimo/doom3.gpl/bl...
Depends on: 712847
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-23 11:23 PST by David Anderson [:dvander]
Modified: 2012-02-01 13:59 PST (History)
10 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't restyle based on state selectors that match our node but don't depend on the state that's changing. (4.04 KB, patch)
2011-11-28 13:43 PST, Boris Zbarsky [:bz]
bzbarsky: review-
Details | Diff | Splinter Review
This is better (4.36 KB, patch)
2011-11-29 10:51 PST, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
This is much simpler (9.15 KB, patch)
2011-12-02 20:03 PST, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-11-23 11:23:41 PST
Firefox freezes trying to select text on this page. It eventually recovers but is pretty unusable. Opera, Safari, IE, and Chrome seem to do fine.

(I tested on 8.0.1)
Comment 1 Boris Zbarsky [:bz] 2011-11-23 11:32:54 PST
Assuming I measured correctly, I see just about all the time under ProcesPendingRestyles, with over 80% under RuleHash::EnumerateAllRules (5% self time, 9% calling SelectorMathes, 67% calling SelectorMatchesTree).

So there are some selectors that take a bit of time to match here... but what are we restyling and why?
Comment 2 Boris Zbarsky [:bz] 2011-11-23 11:40:43 PST
So the stylesheet at https://a248.e.akamai.net/assets.github.com/stylesheets/bundles/github-ed04e8b8be3e88286d2fedb5ade5607df0599719.css has some rules that have fairly slow-to-match selectors:

  .date_selector *
  #code_search .search .site *
  .hero .screenographic *
  #repos .repo .title .security *
  #repos .repo .meta table td *
  #repos .repo .diffs .diff *

as well as a ton of other things using descendant combinators.  WebKit has a sort of fast-path for those that we've also talked about adding...

Still looking into what we're restyling and why, though.  Even matching all those selectors on just a few nodes shouldn't take all that long.
Comment 3 Boris Zbarsky [:bz] 2011-11-23 11:47:27 PST
Oh, and of course the 20% time outside selector matching also indicates that we're recomputing style on more than just a handful of elements!
Comment 4 Boris Zbarsky [:bz] 2011-11-28 13:10:16 PST
Hmm.  So I found one issue sort of.  This rule:

  tr:hover .line-comments{background:#fafafa!important;}

makes every time I move the mouse over the window really slow.  I commented that out, but that didn't help with the selection issue.

Then I commented out this rule:

  #files tr:hover .add-bubble{opacity:1.0;filter:alpha(opacity=100);}

and that made the problem go away.

What seems to be happening is that clicking changes the :active state of the <tr> (which is fine).  So we generate a ContentStateChanged for NS_EVENT_STATE_ACTIVE.  This ends up walking _all_ state selectors and matching them against the element, and if any of them match restyling the element.  Since we are in fact hovering the big table cell that contains all teh code and has a <div id="files"> ancestor, the selector above matches and we think we have to restyle the whole page.  Then the fact that it's got descendant combinators all over kicks in.

So we need to do two things:

1)  Implement that short-circuiting business for descendant combinators.
2)  Fix HasStateDependentStyle to not have this sort of false positive.
Comment 5 Boris Zbarsky [:bz] 2011-11-28 13:11:56 PST
Oh, and maybe:

3)  Change the handling dynamic updates for selectors of the form "A B C D" when the "B" part might have changed to, instead of restyling all descendants of the node just restyling the ones the match "D" or something.  That starts to get complicated...
Comment 6 Boris Zbarsky [:bz] 2011-11-28 13:18:42 PST
So the obvious options for #2 above are to either have lists of state selector per state bit (and only check the bits we care about) or to filter on the bits in HasStateDepedentStyle (before or after) the selector has matched.

The former would use a bunch of memory per RuleCascadeData and complicate the initial setup of the RuleCascadeData a bit.  And these selectors aren't _that_ common.  So for now, doing the filtering seems simpler.
Comment 7 Boris Zbarsky [:bz] 2011-11-28 13:43:50 PST
Created attachment 577357 [details] [diff] [review]
Don't restyle based on state selectors that match our node but don't depend on the state that's changing.

This makes selection pretty fast for me, and the :hover lag is not really noticeable in an opt build, even while being really bad in debug.

I can see the argument for putting the check earlier in the if.... maybe after SelectorMatches but before SelectorMatchesTree?
Comment 8 Boris Zbarsky [:bz] 2011-11-28 14:02:02 PST
I filed bug 705877 on #1 above.
Comment 9 Boris Zbarsky [:bz] 2011-11-29 09:43:18 PST
Comment on attachment 577357 [details] [diff] [review]
Don't restyle based on state selectors that match our node but don't depend on the state that's changing.

This is wrong because it doesn't walk mNegations; luckily we have tests for that thanks to Mounir!
Comment 10 Boris Zbarsky [:bz] 2011-11-29 10:51:40 PST
Created attachment 577672 [details] [diff] [review]
This is better
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-02 15:46:02 PST
Comment on attachment 577672 [details] [diff] [review]
This is better

I think you should probably expand this comment here, since it's now
more than "states that should NOT be tested":

>struct NodeMatchContext {
>  // In order to implement nsCSSRuleProcessor::HasStateDependentStyle,
>  // we need to be able to see if a node might match an
>  // event-state-dependent selector for any value of that event state.
>  // So mStateMask contains the states that should NOT be tested.


SelectorRelevantForStates needs to handle :-moz-any (for each negation).
Please add a test for this if there isn't one already.

r=dbaron with that
Comment 12 Boris Zbarsky [:bz] 2011-12-02 17:41:22 PST
Hmm.  SelectorRelevantForStates doesn't handle :-moz-any, I think.  And the setup in AddSelector is pretty weird for :-moz-any; if state selectors are present under :-moz-any the same selector can end up getting added to mStateSelectors multiple times.

David, how would you feel about mStateSelectors storing (nsCSSSelector*, nsEventStates) pairs so we compute the relevant states once (in AddSelector) and don't have to grovel through the selector here?
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-02 19:08:17 PST
That sounds fine.
Comment 14 Boris Zbarsky [:bz] 2011-12-02 20:03:08 PST
Created attachment 578806 [details] [diff] [review]
This is much simpler

Now with the -moz-any test (which the old patch did fail)
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-02 21:26:38 PST
Comment on attachment 578806 [details] [diff] [review]
This is much simpler

r=dbaron

(And, oh, look, a structure that has holes in 32-bit but packs well in 64-bit.)
Comment 16 Boris Zbarsky [:bz] 2011-12-02 23:16:07 PST
Yeah, I wasn't quite sure what to do about that short of having two separate arrays or something....
Comment 18 Ed Morley [:emorley] 2011-12-06 10:05:34 PST
https://hg.mozilla.org/mozilla-central/rev/5c0c7af66ec0
Comment 19 dindog 2011-12-07 20:52:14 PST
Are there bugs for GitHub slow response of right click? 
Selecting text is fixed, but context menu and mouse gesture still lags severely.
Comment 20 Boris Zbarsky [:bz] 2011-12-07 21:53:34 PST
Separate bugs, please, and cc me?  Make sure to include clear steps to reproduce; I'm not seeing any lag on context menu when I try it.
Comment 21 dindog 2011-12-07 23:05:23 PST
(In reply to Boris Zbarsky (:bz) from comment #20)
> Separate bugs, please, and cc me?  Make sure to include clear steps to
> reproduce; I'm not seeing any lag on context menu when I try it.

I found it cause by an addon name "Firegesture", when turn on the option  drawing the trail of mouse on screen. 

Should I file a bug in bugzilla, or report to the addon author?
Comment 22 Boris Zbarsky [:bz] 2011-12-07 23:25:18 PST
Ideally, both.

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