The default bug view has changed. See this FAQ.

Unusable performance selecting text on github

RESOLVED FIXED in mozilla11

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: bz)

Tracking

unspecified
mozilla11
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)
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?
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.
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!
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.
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...
Component: Selection → Style System (CSS)
QA Contact: selection → style-system
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.
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?
Attachment #577357 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
I filed bug 705877 on #1 above.
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!
Attachment #577357 - Flags: review?(dbaron) → review-
Created attachment 577672 [details] [diff] [review]
This is better
Attachment #577672 - Flags: review?(dbaron)
Attachment #577357 - Attachment is obsolete: true
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
Attachment #577672 - Flags: review?(dbaron) → review+
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?
That sounds fine.
Created attachment 578806 [details] [diff] [review]
This is much simpler

Now with the -moz-any test (which the old patch did fail)
Attachment #578806 - Flags: review?(dbaron)
Attachment #577672 - Attachment is obsolete: true
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.)
Attachment #578806 - Flags: review?(dbaron) → review+
Yeah, I wasn't quite sure what to do about that short of having two separate arrays or something....
Whiteboard: [need review] → [need landing]
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0c7af66ec0
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/5c0c7af66ec0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 19

5 years ago
Are there bugs for GitHub slow response of right click? 
Selecting text is fixed, but context menu and mouse gesture still lags severely.
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

5 years ago
(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?
Ideally, both.
Depends on: 712847
You need to log in before you can comment on or make changes to this bug.