RuleSet should use malloc rather than Vector

RuleSet should use malloc rather than Vector
    
The lion's share of the memory used by the style resolver is in the RuleSet
objects. Prior to this CL, these objects were structured as a HashMap from
AtomicStrings to pointers to vectors of RuleData. This CL simplifies this
object graph by removing a layer of indirection. Now we just have a HashMap
from AtomicStrings to an array of RuleDatas.

Rather than use a length to terminate the iteration over the Vector, this CL
uses a bit in RuleData to mark the end of the array. Together with removing the
extra pointer, this CL saves 15 kB on Mobile Gmail.

R=eseidel,ojan

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

[eseidel, 2013-06-26 00:26:57]

lgtm

[commit-bot: I haz the power, 2013-06-26 00:27:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/17616009/5001

[esprehn, 2013-06-26 00:37:33]

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
Source/core/css/ElementRuleCollector.cpp:258: collectRuleIfMatches(*rules++,
matchRequest, ruleRange);
you could maybe do/while {} but I don't know if that's cleaner.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
Source/core/css/ElementRuleCollector.cpp:259: collectRuleIfMatches(*rules++,
matchRequest, ruleRange);
No reason to increment here.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cpp
File Source/core/css/RuleSet.cpp (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:137: class TerminatedArrayBuilder {
I'd rather we added a TerminatedArray<T> class with size() that does the loop
you have below in the constructor and exposed a proper ::iterator and such, but
we can sort that out in the future.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:170: m_array.get()[m_count++] = item;
This looks easy to break, I can append() an item that claims it's the last.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:205: , m_isLastInArray(0)
I think you can just do false here.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h#...
Source/core/css/RuleSet.h:65: void setLastInArray(bool flag) { m_isLastInArray =
flag ? 1 : 0; }
I don't think you need the ternary in here.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h#...
Source/core/css/RuleSet.h:123: const RuleData*
shadowPseudoElementRules(AtomicStringImpl* key) const { ASSERT(!m_pendingRules);
return m_shadowPseudoElementRules.get(key); }
We might consider a WTF class someday that does the isLastElement() check
internal to the MarkedArray<RuleData>::iterator.

[abarth-chromium, 2013-06-26 00:57:00]

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
Source/core/css/ElementRuleCollector.cpp:258: collectRuleIfMatches(*rules++,
matchRequest, ruleRange);
On 2013/06/26 00:37:33, esprehn wrote:
> you could maybe do/while {} but I don't know if that's cleaner.

Yeah, I tried a few permutations of this loop, and this one seemed the cleanest.
 I'm open to suggestions if you think something else would be better.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/ElementRul...
Source/core/css/ElementRuleCollector.cpp:259: collectRuleIfMatches(*rules++,
matchRequest, ruleRange);
On 2013/06/26 00:37:33, esprehn wrote:
> No reason to increment here.

Done.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cpp
File Source/core/css/RuleSet.cpp (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:137: class TerminatedArrayBuilder {
On 2013/06/26 00:37:33, esprehn wrote:
> I'd rather we added a TerminatedArray<T> class with size() that does the loop
> you have below in the constructor and exposed a proper ::iterator and such,
but
> we can sort that out in the future.

Ok.  I struggled with how much to abstract this work.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:170: m_array.get()[m_count++] = item;
On 2013/06/26 00:37:33, esprehn wrote:
> This looks easy to break, I can append() an item that claims it's the last.

There's an ASSERT in release() to make sure the array is consistent when we're
done building.  I've added an ASSERT here as well.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:205: , m_isLastInArray(0)
On 2013/06/26 00:37:33, esprehn wrote:
> I think you can just do false here.

Done.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h#...
Source/core/css/RuleSet.h:65: void setLastInArray(bool flag) { m_isLastInArray =
flag ? 1 : 0; }
On 2013/06/26 00:37:33, esprehn wrote:
> I don't think you need the ternary in here.

Done.

https://codereview.chromium.org/17616009/diff/2001/Source/core/css/RuleSet.h#...
Source/core/css/RuleSet.h:123: const RuleData*
shadowPseudoElementRules(AtomicStringImpl* key) const { ASSERT(!m_pendingRules);
return m_shadowPseudoElementRules.get(key); }
On 2013/06/26 00:37:33, esprehn wrote:
> We might consider a WTF class someday that does the isLastElement() check
> internal to the MarkedArray<RuleData>::iterator.

Yeah.

[commit-bot: I haz the power, 2013-06-26 00:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/17616009/25003

[esprehn, 2013-06-26 00:58:42]

LGTM.

[commit-bot: I haz the power, 2013-06-26 02:41:02]

Change committed as 153042