Support invalidation sets for simple class selectors.

Support invalidation sets for simple class selectors.

Invalidation sets were supported for selectors like '.a .b' with an empty
invalidation set for 'b' and a LocalStyleChange for elements for which 'b'
was added or removed. However, not for simple class selectors like '.b' for
which an empty invalidation set would be added, but also marked with
wholeSubtreeInvalid yielding a SubtreeStyleChange instead.

Now, start using invalidation sets for class selectors in the rightmost
compound selector even when there are no other class selectors present.

R=chrishtr@chromium.org,ojan@chromium.org
BUG=335247
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167470

[chrishtr, 2014-02-19 17:50:44]

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature.cpp
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:56: bool foundDescendantRelation = false;
remove these variables

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:66: foundIdent = true;
return true, remove if (!foundDescendantRelation)

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:74: foundDescendantRelation = true;
return true;

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:82: return foundIdent;
remove this line

[rune, 2014-02-19 18:47:40]

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature.cpp
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:56: bool foundDescendantRelation = false;
On 2014/02/19 17:50:44, Chris Harrelson wrote:
> remove these variables

See other comment.

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:66: foundIdent = true;
On 2014/02/19 17:50:44, Chris Harrelson wrote:
> return true, remove if (!foundDescendantRelation)

We still need to continue to look for adjacent combinators and
!isSkippableComponentForInvalidation() for which we need to return false even if
foundIdent is true.

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:82: return foundIdent;
On 2014/02/19 17:50:44, Chris Harrelson wrote:
> remove this line

See other comment.

[chrishtr, 2014-02-19 18:54:00]

lgtm

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature.cpp
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/171513006/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:66: foundIdent = true;
On 2014/02/19 18:47:40, rune - CET wrote:
> On 2014/02/19 17:50:44, Chris Harrelson wrote:
> > return true, remove if (!foundDescendantRelation)
> 
> We still need to continue to look for adjacent combinators and
> !isSkippableComponentForInvalidation() for which we need to return false even
if
> foundIdent is true.

Oh right, good point.

[ojan, 2014-02-19 19:40:46]

lgtm

[ojan, 2014-02-19 19:41:16]

esprehn, FYI

[esprehn, 2014-02-19 19:49:15]

On 2014/02/19 19:41:16, ojan wrote:
> esprehn, FYI

lgtm, we should really optimize the memory usage of DescendantInvalidationSet
again too. I had originally used OwnPtr instead of HashTable in there, but
chrishtr undid my optimization. Now G+ and AdBlock will use 650kb more memory. I
guess that's not huge, but it's something to think about.

[rune, 2014-02-19 20:33:42]

On 2014/02/19 19:49:15, esprehn wrote:
> On 2014/02/19 19:41:16, ojan wrote:
> > esprehn, FYI
> 
> lgtm, we should really optimize the memory usage of DescendantInvalidationSet
> again too. I had originally used OwnPtr instead of HashTable in there, but
> chrishtr undid my optimization. Now G+ and AdBlock will use 650kb more memory.
I
> guess that's not huge, but it's something to think about.

Ouch, I was suspecting that the large number of HashTables would amount to a lot
of memory. This CL should not increase the usage, though, since
collectFeaturesFromSelector also calls ensureClassInvalidationSet and calls
setWholeSubtreeInvalid() on it. It's particularly bad that this increased memory
use will ship after the classesInRules HashSet was replaced for invalidation
sets even when TargetedStyleRecalc is disabled.

[rune, 2014-02-19 20:34:13]

The CQ bit was unchecked by rune@opera.com

[rune, 2014-02-19 20:34:16]

The CQ bit was checked by rune@opera.com

[chrishtr, 2014-02-19 21:40:10]

Good point about the memory, I'll send a patch for that.


On Wed, Feb 19, 2014 at 12:33 PM, <rune@opera.com> wrote:

> On 2014/02/19 19:49:15, esprehn wrote:
>
>> On 2014/02/19 19:41:16, ojan wrote:
>> > esprehn, FYI
>>
>
>  lgtm, we should really optimize the memory usage of
>> DescendantInvalidationSet
>> again too. I had originally used OwnPtr instead of HashTable in there, but
>> chrishtr undid my optimization. Now G+ and AdBlock will use 650kb more
>> memory.
>>
> I
>
>> guess that's not huge, but it's something to think about.
>>
>
> Ouch, I was suspecting that the large number of HashTables would amount to
> a lot
> of memory. This CL should not increase the usage, though, since
> collectFeaturesFromSelector also calls ensureClassInvalidationSet and calls
> setWholeSubtreeInvalid() on it. It's particularly bad that this increased
> memory
> use will ship after the classesInRules HashSet was replaced for
> invalidation
> sets even when TargetedStyleRecalc is disabled.
>
>
> https://codereview.chromium.org/171513006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-02-19 22:02:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/171513006/1

[commit-bot: I haz the power, 2014-02-19 23:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/171513006/1

[commit-bot: I haz the power, 2014-02-20 07:32:23]

Change committed as 167470