86434 2012-05-14 21:40:53 -0700 RuleSet::addToRuleSet wastes a bit of Vector capacity 2012-05-15 10:03:59 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 86281 1 simon.fraser webkit-unassigned darin kling koivisto macpherson menard simon.fraser webkit.review.bot oldest_to_newest 623625 0 simon.fraser 2012-05-14 21:40:53 -0700 Data collected via bug 86281 show that RuleSet::addToRuleSet() is the most wasteful call site for excess vector capacity. 623629 1 kling 2012-05-14 21:44:18 -0700 Hm. This is somewhat surprising since we shrinkToFit() most of the vectors in RuleSet::shrinkToFit(). Something must be going wrong here. :/ 623790 2 koivisto 2012-05-15 01:30:28 -0700 Simon, does your logging work correctly when shrinkToFit is used? 624100 3 simon.fraser 2012-05-15 08:14:07 -0700 Apparently not (you can see the patch in bug 86281). 624140 4 simon.fraser 2012-05-15 08:44:31 -0700 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 624151 5 141983 kling 2012-05-15 08:52:28 -0700 Created attachment 141983 Proposed patch 624163 6 141983 koivisto 2012-05-15 09:09:01 -0700 Comment on attachment 141983 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". 624189 7 kling 2012-05-15 09:45:35 -0700 Committed r117084: <http://trac.webkit.org/changeset/117084> 624192 8 darin 2012-05-15 09:50:22 -0700 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? 624195 9 darin 2012-05-15 09:54:45 -0700 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. 624198 10 kling 2012-05-15 09:58:42 -0700 (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. 624204 11 darin 2012-05-15 10:03:28 -0700 OK. Got it. 624205 12 darin 2012-05-15 10:03:59 -0700 One last thought on this: The prediction can be wrong some of the time as long as it’s right most of the time. 141983 2012-05-15 08:52:28 -0700 2012-05-15 09:09:00 -0700 Proposed patch bug-86434.diff text/plain 1331 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3YmJjNzI5Li40YjQ1NTYwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTItMDUtMTUgIEFuZHJlYXMgS2xpbmcgIDxrbGluZ0B3ZWJraXQub3JnPgorCisgICAg ICAgIFJ1bGVTZXQ6OmFkZFRvUnVsZVNldCB3YXN0ZXMgYSBsb3Qgb2YgVmVjdG9yIGNhcGFjaXR5 LgorICAgICAgICA8aHR0cDovL3dlYmtpdC5vcmcvYi84NjQzND4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBIYXZlIG1ha2VSdWxlU2V0IGNhbGwgc2hy aW5rVG9GaXQoKSBvbiB0aGUgdmVjdG9ycyBiZWZvcmUgcmV0dXJuaW5nIHRoZW0uCisgICAgICAg IFRoaXMgYXZvaWRzIHdhc3Rpbmcgc3BhY2UgZm9yIHRoZSBydWxlc2V0cyBjb25zdHJ1Y3RlZCBp biBjb2xsZWN0RmVhdHVyZXMoKS4KKworICAgICAgICAqIGNzcy9TdHlsZVJlc29sdmVyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6Om1ha2VSdWxlU2V0KToKKwogMjAxMi0wNS0xNSAgSm9jZWx5biBU dXJjb3R0ZSAgPGpvY2VseW4udHVyY290dGVAbm9raWEuY29tPgogCiAgICAgICAgIFtRdF0gaHR0 cC90ZXN0cy94bWxodHRwcmVxdWVzdC94bWxodHRwcmVxdWVzdC1jaGVjay1oZWFkLXJlYWR5c3Rh dGUtZm9yLTQwNC5odG1sIHRpbWVzIG91dApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvY3Nz L1N0eWxlUmVzb2x2ZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3Bw CmluZGV4IDhiNWJkYjMuLmUxYjAwZmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9T dHlsZVJlc29sdmVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5j cHAKQEAgLTQ2MCw2ICs0NjAsNyBAQCBzdGF0aWMgUGFzc093blB0cjxSdWxlU2V0PiBtYWtlUnVs ZVNldChjb25zdCBWZWN0b3I8U3R5bGVSZXNvbHZlcjo6UnVsZUZlYXR1cmU+JgogICAgIE93blB0 cjxSdWxlU2V0PiBydWxlU2V0ID0gUnVsZVNldDo6Y3JlYXRlKCk7CiAgICAgZm9yIChzaXplX3Qg aSA9IDA7IGkgPCBzaXplOyArK2kpCiAgICAgICAgIHJ1bGVTZXQtPmFkZFJ1bGUocnVsZXNbaV0u cnVsZSwgcnVsZXNbaV0uc2VsZWN0b3IsIHJ1bGVzW2ldLmhhc0RvY3VtZW50U2VjdXJpdHlPcmln aW4sIGZhbHNlKTsKKyAgICBydWxlU2V0LT5zaHJpbmtUb0ZpdCgpOwogICAgIHJldHVybiBydWxl U2V0LnJlbGVhc2UoKTsKIH0KIAo=