We could save a lot of memory by making StylePropertySet allocate exactly the amount of space it needs for CSSProperty objects at construction time (instead of using a Vector.) We'd still have to switch over to Vector mode when the StylePropertySet is mutated, but that won't happen for the majority of them.
Created attachment 144080 [details] Patch me if you can!
Comment on attachment 144080 [details] Patch me if you can! View in context: https://bugs.webkit.org/attachment.cgi?id=144080&action=review Looks like an excellent memory win. > Source/WebCore/css/StylePropertySet.h:155 > + Vector<CSSProperty>* m_vector; > + > friend class PropertySetCSSStyleDeclaration; > }; As discussed one potential trick would be have separate subclasses for inlined properties and Vector cases. The subclasses would never be instantiated but they would be static_casted to. This would avoid reinterpret_casts and also make semantically some sense as we really have sort-of subclasses (switched dynamically in place). If that doesn't work out just doing something like union { Vector<CSSProperty>* m_vector; void* m_properties; } could at least avoid weird casts from Vector* -> CSSProperty (CSSProperty is a complex type so can't be used in union).
Created attachment 144548 [details] I met my wife on patch.com
Comment on attachment 144548 [details] I met my wife on patch.com View in context: https://bugs.webkit.org/attachment.cgi?id=144548&action=review Nice, r=me. > Source/WebCore/css/StylePropertySet.cpp:64 > + RefPtr<StylePropertySet> result = adoptRef(new StylePropertySet(CSSStrictMode)); > + *result->m_vector = properties; > + return result.release(); > +} Usually the vector would be passed in as constructor parameter instead of being set in create(). Mutability bit could be passed in as enum. > Source/WebCore/css/StylePropertySet.cpp:120 > + if (other.isMutable()) > + *m_vector = *other.m_vector; > + else { Use early return. > Source/WebCore/css/StylePropertySet.h:58 > + inline unsigned propertyCount() const; > + inline bool isEmpty() const; > + inline const CSSProperty& propertyAt(unsigned index) const; > + inline CSSProperty& propertyAt(unsigned index); The inline keywords should go to the definition. They are not needed on declaration. > Source/WebCore/css/StylePropertySet.h:152 > + unsigned m_arraySize : 28; > + > + union { > + Vector<CSSProperty>* m_vector; > + void* m_properties; > + }; m_vector could use a more descriptive name. m_mutablePropertyVector or something?
Committed r118809: <http://trac.webkit.org/changeset/118809>
Comment on attachment 144548 [details] I met my wife on patch.com View in context: https://bugs.webkit.org/attachment.cgi?id=144548&action=review > Source/WebCore/css/StylePropertySet.cpp:55 > + void* slot = WTF::fastMalloc(sizeof(StylePropertySet) - sizeof(void*) + sizeof(CSSProperty) * properties.size()); Should not need a WTF prefix here. How does this work when we deref the last reference? I don’t think delete can correctly delete something if it’s made directly with fastMalloc rather than with operator new, but maybe I just missed something.
(In reply to comment #6) > (From update of attachment 144548 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144548&action=review > > > Source/WebCore/css/StylePropertySet.cpp:55 > > + void* slot = WTF::fastMalloc(sizeof(StylePropertySet) - sizeof(void*) + sizeof(CSSProperty) * properties.size()); > > Should not need a WTF prefix here. Fair. > How does this work when we deref the last reference? I don’t think delete can correctly delete something if it’s made directly with fastMalloc rather than with operator new, but maybe I just missed something. StylePropertySet still inherits from RefCounted and so gets the proper "operator delete" implementation from there.
(In reply to comment #6) > How does this work when we deref the last reference? I don’t think delete can correctly delete something if it’s made directly with fastMalloc rather than with operator new, but maybe I just missed something. Good point, I think it works by luck basically. The right thing to do is to override deref() and pair fastMalloc with fastFree based on the mutability bit.
We were wondering how do you measure these memory usage numbers, and what 500kB on wsj.com is like as a percentage of overall memory usage? The ChangeLog doesn't explain where the memory saving comes from, or how much it achieves per StylePropertySet, and I'd like to understand those things. Thanks!
I suppose this doesn't actually make any practical difference as WTF_MAKE_FAST_ALLOCATED makes us invoke fastFree() on delete anyway.
(In reply to comment #9) > We were wondering how do you measure these memory usage numbers, and what 500kB on wsj.com is like as a percentage of overall memory usage? > > The ChangeLog doesn't explain where the memory saving comes from, or how much it achieves per StylePropertySet, and I'd like to understand those things. > > Thanks! Heya! Removing the vector from StylePropertySet gets rid of two size_t's (vector size and capacity) as well as the inline capacity buffer (4 * sizeof(CSSProperty)) and replaces this by an array tacked onto the end of StylePropertySet. In other words, an immutable 2-property StylePropertySet goes from 88 bytes (64-bit numbers here): bitfield (8 bytes) vector size+capacity (16 bytes) vector inline buffer (64 bytes) ...to 40 bytes: bitfield (8 bytes) property array (32 bytes) The memory usage numbers came from some ad-hoc instrumenting to count the number of live StylePropertySets on a page, and the space allocated by each one. Before this patch, they had a total mass of ~6.5MB on wsj.com and after we were down to ~6.0MB. Numbers vary slightly between loads, I just like to include some value as "patch marketing." In retrospect, it would have been even better marketing to spell out the percentage saved, etc. Note that I haven't taken allocator overhead into account, which is a lower-level issue and while no less important, an entirely separate problem.
(In reply to comment #10) > I suppose this doesn't actually make any practical difference as WTF_MAKE_FAST_ALLOCATED makes us invoke fastFree() on delete anyway. Right. Given that WTF_MAKE_FAST_ALLOCATED unconditionally expands to fastMalloc/fastFree calls which are then substituted by regular malloc()/free() in FastMalloc in the case of USE_SYSTEM_MALLOC, do you still feel that we should put a copy-pasted operator delete override here?
(In reply to comment #11) > The memory usage numbers came from some ad-hoc instrumenting to count the number of live StylePropertySets on a page, and the space allocated by each one. Before this patch, they had a total mass of ~6.5MB on wsj.com and after we were down to ~6.0MB. Numbers vary slightly between loads, I just like to include some value as "patch marketing." In retrospect, it would have been even better marketing to spell out the percentage saved, etc. So if you save 48 bytes per object, that must mean there are something like ten thousand style property sets on that page. That seems like a surprisingly high number of StylePropertySets to create for a page. Could it be that the underlying allocator is going by powers of two, and that by decreasing the inline buffer size to two instead of four you'll fall into the same allocator bucket and have the same effective memory usage without having to go to the trouble of switching between a vector and an array?