704911 - Unusable performance selecting text on github

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

I filed bug 705877 on #1 above.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: This is better

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

That sounds fine.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: This is much simpler

Now with the -moz-any test (which the old patch did fail)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 [:bzbarsky]]

Yeah, I wasn't quite sure what to do about that short of having two separate arrays or something....

Comment 17

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0c7af66ec0

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c0c7af66ec0

Comment 19

[dindog]

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 [:bzbarsky]]

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]

(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 [:bzbarsky]]

Ideally, both.