Avoid looking at invalidation sets in a SubtreeStyleChange subtree

Avoid looking at invalidation sets in a SubtreeStyleChange subtree

We were matching all the rulesets in a subtree even after finding a ruleset
that would invalidate the whole subtree which is wasted work. We were also
storing all the invalidation sets for an element even after one of them
would have invalidated the entire subtree.

This patch makes us only ever store one invalidation set for an element once
one of them would invalidate the whole subtree. It also removes the
foundInvalidationSet() bit since that's identical to just checking if the
Vector of invalidation sets is non-empty.

I also removed an ASSERT(renderer->style()), all renderers have a style, so
there's no reason to check this.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171417

[rune, 2014-04-11 20:40:12]

lgtm

https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:103:
m_recursionData.pushInvalidationSet(**it);

You introduce some invariants here about invalidation sets not being pushed when
you have wholeSubtreeInvalid and invalidationList for an element having a single
set if it's wholeSubtreeInvalid, hence no use of  break here. Perhaps an ASSERT
for m_recursionData.wholeSubtreeInvalid() still being false before the push?

[esprehn, 2014-04-11 20:50:49]

On 2014/04/11 20:40:12, rune - CET wrote:
> lgtm
> 
>
https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
> File Source/core/css/invalidation/StyleInvalidator.cpp (right):
> 
>
https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
> Source/core/css/invalidation/StyleInvalidator.cpp:103:
> m_recursionData.pushInvalidationSet(**it);
> 
> You introduce some invariants here about invalidation sets not being pushed
when
> you have wholeSubtreeInvalid and invalidationList for an element having a
single
> set if it's wholeSubtreeInvalid, hence no use of  break here. Perhaps an
ASSERT
> for m_recursionData.wholeSubtreeInvalid() still being false before the push?

I'm not sure that's a useful ASSERT, the early return at the top of the function
deals with it already. If we did push it again that wouldn't be an issue though,
it's just wasted work.

[rune, 2014-04-11 20:54:35]

On 2014/04/11 20:50:49, esprehn wrote:
> On 2014/04/11 20:40:12, rune - CET wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
> > File Source/core/css/invalidation/StyleInvalidator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/233243003/diff/20001/Source/core/css/invalida...
> > Source/core/css/invalidation/StyleInvalidator.cpp:103:
> > m_recursionData.pushInvalidationSet(**it);
> > 
> > You introduce some invariants here about invalidation sets not being pushed
> when
> > you have wholeSubtreeInvalid and invalidationList for an element having a
> single
> > set if it's wholeSubtreeInvalid, hence no use of  break here. Perhaps an
> ASSERT
> > for m_recursionData.wholeSubtreeInvalid() still being false before the push?
> 
> I'm not sure that's a useful ASSERT, the early return at the top of the
function
> deals with it already. If we did push it again that wouldn't be an issue
though,
> it's just wasted work.

sure.

[esprehn, 2014-04-14 03:05:38]

I added an ASSERT to pushInvalidationSet, you're right it's a good thing to
have.

[esprehn, 2014-04-14 03:06:09]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-14 03:06:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/233243003/40001

[commit-bot: I haz the power, 2014-04-14 03:40:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-14 03:40:14]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[esprehn, 2014-04-14 04:32:46]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-14 04:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/233243003/40001

[commit-bot: I haz the power, 2014-04-14 05:03:39]

Change committed as 171417