Bug 87506 - Make StylePropertySet a variable-sized object to reduce memory use.
Summary: Make StylePropertySet a variable-sized object to reduce memory use.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-25 09:02 PDT by Andreas Kling
Modified: 2012-05-30 16:33 PDT (History)
6 users (show)

See Also:


Attachments
Patch me if you can! (16.71 KB, patch)
2012-05-25 09:13 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
I met my wife on patch.com (18.77 KB, patch)
2012-05-29 07:01 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-05-25 09:02:54 PDT
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.
Comment 1 Andreas Kling 2012-05-25 09:13:34 PDT
Created attachment 144080 [details]
Patch me if you can!
Comment 2 Antti Koivisto 2012-05-25 10:12:57 PDT
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).
Comment 3 Andreas Kling 2012-05-29 07:01:18 PDT
Created attachment 144548 [details]
I met my wife on patch.com
Comment 4 Antti Koivisto 2012-05-29 07:44:05 PDT
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?
Comment 5 Andreas Kling 2012-05-29 11:20:28 PDT
Committed r118809: <http://trac.webkit.org/changeset/118809>
Comment 6 Darin Adler 2012-05-29 13:25:35 PDT
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.
Comment 7 Andreas Kling 2012-05-29 13:29:45 PDT
(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.
Comment 8 Antti Koivisto 2012-05-29 14:55:33 PDT
(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.
Comment 9 Luke Macpherson 2012-05-29 23:01:15 PDT
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!
Comment 10 Antti Koivisto 2012-05-30 06:27:22 PDT
I suppose this doesn't actually make any practical difference as WTF_MAKE_FAST_ALLOCATED makes us invoke fastFree() on delete anyway.
Comment 11 Andreas Kling 2012-05-30 07:46:20 PDT
(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.
Comment 12 Andreas Kling 2012-05-30 07:54:07 PDT
(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?
Comment 13 Luke Macpherson 2012-05-30 16:33:33 PDT
(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?