Bug 86434 – RuleSet::addToRuleSet wastes a bit of Vector capacity

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Data collected via bug 86281 show that RuleSet::addToRuleSet() is the most wasteful call site for excess vector capacity.

[Andreas Kling]

Hm. This is somewhat surprising since we shrinkToFit() most of the vectors in RuleSet::shrinkToFit(). Something must be going wrong here. :/

[Antti Koivisto]

Simon, does your logging work correctly when shrinkToFit is used?

[Simon Fraser (smfr)]

Apparently not (you can see the patch in bug 86281).

[Simon Fraser (smfr)]

Previous data was not taking shrinkToFit into account. New data shows addToRuleSet() to be a lot less wasteful, but there is one stack that shows up in the top 20:

117 vectors, 7.11KB used of 73.12KB, 66.02KB wasted at:
1   0x108c1a255 WTF::Vector<WebCore::RuleData, 0ul>::Vector()
2   0x108c07a55 WTF::Vector<WebCore::RuleData, 0ul>::Vector()
3   0x108bfb4e1 WebCore::RuleSet::addToRuleSet(WTF::AtomicStringImpl*, WTF::HashMap<WTF::AtomicStringImpl*, WTF::OwnPtr<WTF::Vector<WebCore::RuleData, 0ul> >, WTF::PtrHash<WTF::AtomicStringImpl*>, WTF::HashTraits<WTF::AtomicStringImpl*>, WTF::HashTraits<WTF::OwnPtr<WTF::Vector<WebCore::RuleData, 0ul> > > >&, WebCore::RuleData const&)
4   0x108bfb6fb WebCore::RuleSet::addRule(WebCore::StyleRule*, WebCore::CSSSelector*, bool, bool, bool)
5   0x108beac71 _ZN7WebCoreL11makeRuleSetERKN3WTF6VectorINS_13StyleResolver11RuleFeatureELm0EEE

[Andreas Kling]

Created attachment 141983 [details]
Proposed patch

[Antti Koivisto]

Comment on attachment 141983 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141983&action=review

r=me

> Source/WebCore/ChangeLog:3
> +        RuleSet::addToRuleSet wastes a lot of Vector capacity.

You might want to update the title. This hardly wastes "a lot".

[Andreas Kling]

Committed r117084: <http://trac.webkit.org/changeset/117084>

[Darin Adler]

In cases like this where we know the size the RuleSet will be before we create it, it would be more efficient to reserve capacity than to shrink to fit. All we’d have to do is make a RuleSet::create function overload that takes a capacity.

What do you guys think of that idea?

[Darin Adler]

Well maybe this is the only call site like that; the point is that you save more memory by not allocating the larger block and shrinking. I suspect that in many cases the memory saved by shrinking after the fact can’t be reused due to fragmentation.

[Andreas Kling]

(In reply to comment #8)
> In cases like this where we know the size the RuleSet will be before we create it, it would be more efficient to reserve capacity than to shrink to fit. All we’d have to do is make a RuleSet::create function overload that takes a capacity.
> 
> What do you guys think of that idea?

I like the idea in principle. If you look at the implementation of RuleSet::shrinkToFit() however, it's nontrivial to predict at RuleSet::create() time how big each subvector should be.

[Darin Adler]

OK. Got it.

[Darin Adler]

One last thought on this: The prediction can be wrong some of the time as long as it’s right most of the time.