Bug 22717 - Make CSS values use less memory
Summary: Make CSS values use less memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-06 21:09 PST by Antti Koivisto
Modified: 2010-03-27 13:04 PDT (History)
3 users (show)

See Also:


Attachments
Get CSSValue off from StyleBase (9.40 KB, patch)
2008-12-06 22:07 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
share primitive value instances (5.63 KB, patch)
2008-12-07 15:12 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2008-12-06 21:09:13 PST
CSS values have lots room for optimisation
Comment 1 Antti Koivisto 2008-12-06 22:07:25 PST
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.
Comment 2 Antti Koivisto 2008-12-07 15:12:54 PST
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.
Comment 3 Darin Adler 2008-12-07 15:47:13 PST
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.
Comment 4 Darin Adler 2008-12-07 15:49:58 PST
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.
Comment 5 Darin Adler 2008-12-07 15:57:19 PST
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
Comment 6 Antti Koivisto 2008-12-07 17:36:07 PST
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.
Comment 7 Antti Koivisto 2008-12-07 17:36:28 PST
Sending        WebCore/ChangeLog
Sending        WebCore/css/CSSPrimitiveValue.cpp
Sending        WebCore/css/CSSPrimitiveValue.h
Transmitting file data ...
Committed revision 39088.

Comment 8 Mark Rowe (bdash) 2010-03-27 13:04:30 PDT
This change caused bug 31223.