Bug 22717 – Make CSS values use less memory

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

[Antti Koivisto]

CSS values have lots room for optimisation

[Antti Koivisto]

Created attachment 25827 [details]
Get CSSValue off from StyleBase

Disabled (instead of refactoring around the lack of comon base) the ability to have style declaration blocks as CSS variable values. They don't exist in the spec so I wasn't sure if they have future or not. It would not be hard to get them back. CSS variabled are in any case an experimental feature and not enabled by default.

[Antti Koivisto]

Created attachment 25832 [details]
share primitive value instances

A more stylish solution than sharing would be to turn CSSPrimitiveValue (or CSSValues in general) into non-virtual, non-refcounted simple type with value semantics. In practice these sharing tricks get similar memory benefits with less need for refactoring.

This does not directly depend on the previous patch (as the parent field of CSSValues is never used) thought it depends on it conceptually to facilitate sharing.

[Darin Adler]

I seem to recall that there's no real need for the StyleBase base class in most cases; there is very little polymorphic use that's really needed. I did some research on this a while back and had a series of patches to eliminate StyleBase altogether, although my motivation was entirely theoretical and not practical memory use reduction.

[Darin Adler]

Comment on attachment 25827 [details]
Get CSSValue off from StyleBase

r=me

All those "#if 0" are a little ugly. I'd almost prefer removing the code entirely. But I can live with it.

[Darin Adler]

Comment on attachment 25832 [details]
share primitive value instances

> +    // These are the empty and deleted values of the hash table.
> +    static CSSPrimitiveValue* colorTransparent = new CSSPrimitiveValue(Color::transparent);
> +    static CSSPrimitiveValue* colorWhite = new CSSPrimitiveValue(Color::white);
> +    if (rgbValue == Color::transparent)
> +        return colorTransparent;
> +    if (rgbValue == Color::white)
> +        return colorWhite;

The declarations could go inside the if statements. I think it would be slightly more elegant.

> +    RefPtr<CSSPrimitiveValue> primitiveValue = colorValueCache->get(rgbValue);
> +    if (primitiveValue)
> +        return primitiveValue;

Saves one round of reference count churn if you return primitiveValue.release() in the various places you're returning a RefPtr, including this one.

> +    primitiveValue = adoptRef(new CSSPrimitiveValue(rgbValue));
> +    // Just wipe out the cache and start rebuilding when it gets too big.
> +    const int maxColorCacheSize = 512;
> +    if (colorValueCache->size() > maxColorCacheSize)

This makes the name of the global a lie. You should use ">=" if you really want it to be the max size.

> +    const int maxCachedUnitType = CSS_PX;

The maxCachedUnitType thing here is subtle. I think you need a comment saying something about the fact that CSS_PX is the last one, and maybe even a comment where CSS_PX is defined telling about this dependency.

> +    static RefPtr<CSSPrimitiveValue>(* integerValueCache)[maxCachedUnitType + 1] = new RefPtr<CSSPrimitiveValue>[cachedIntegerCount][maxCachedUnitType + 1];

A typedef would make this way easier to read.

> +    int intValue;
> +    if (type <= CSS_PX && value >= 0 && value < cachedIntegerCount && value == (intValue = (int)value)) {

We normally prefer C++ casts. But I'm thinking that you might want to move that last check into a separate if statement just so you can do the assignment without doing a cast at all.

> +    

I noticed some trailing spaces on some of the blank lines.

> -    static PassRefPtr<CSSPrimitiveValue> create(const String& value, UnitTypes type)
> -    {
> -        return adoptRef(new CSSPrimitiveValue(value, type));
> -    }

Since you left this one alone, you could leave it in the header file. I don't have any strong feelings either way.

All my comments were quibbles, not real objections. r=me

[Antti Koivisto]

Sending        WebCore/ChangeLog
Sending        WebCore/css/CSSInitialValue.h
Sending        WebCore/css/CSSParser.cpp
Sending        WebCore/css/CSSParser.h
Sending        WebCore/css/CSSValue.h
Sending        WebCore/css/CSSVariablesDeclaration.cpp
Sending        WebCore/css/CSSVariablesDeclaration.h
Sending        WebCore/css/StyleBase.h
Transmitting file data ........
Committed revision 39086.

[Antti Koivisto]

Sending        WebCore/ChangeLog
Sending        WebCore/css/CSSPrimitiveValue.cpp
Sending        WebCore/css/CSSPrimitiveValue.h
Transmitting file data ...
Committed revision 39088.

[Mark Rowe (bdash)]

This change caused bug 31223.