[webkit-reviews] review granted: [Bug 22717] Make CSS values use less memory : [Attachment 25832] share primitive value instances

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 7 15:57:20 PST 2008


Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 22717: Make CSS values use less memory
https://bugs.webkit.org/show_bug.cgi?id=22717

Attachment 25832: share primitive value instances
https://bugs.webkit.org/attachment.cgi?id=25832&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // 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


More information about the webkit-reviews mailing list