Bug 71668 – CSSValue: Devirtualize isFooType().

• Component: CSS
• Status: RESOLVED FIXED

[Andreas Kling]

CSSValue: Devirtualize isFooType().

[Andreas Kling]

Created attachment 113847 [details]
Patch (depends on 71667)

[Antti Koivisto]

Comment on attachment 113847 [details]
Patch (depends on 71667)

View in context: https://bugs.webkit.org/attachment.cgi?id=113847&action=review

> Source/WebCore/css/CSSPrimitiveValueMappings.h:1342
> +    : CSSValue(PrimitiveClass, CSS_PRIMITIVE_VALUE, /* isPrimitive */ true)

Kinda hatin' here...

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 113847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113847&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:1342
> > +    : CSSValue(PrimitiveClass, CSS_PRIMITIVE_VALUE, /* isPrimitive */ true)
> 
> Kinda hatin' here...

Reviewers gonna review. I'll redesign as discussed.

[Andreas Kling]

Created attachment 113904 [details]
Proposed patch v2

A little less verbose this time. ;)

[Darin Adler]

Comment on attachment 113904 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=113904&action=review

> Source/WebCore/css/CSSImageValue.h:53
> +    CSSImageValue(const String& url);

This single-argument constructor should probably be marked explicit.

> Source/WebCore/css/CSSMutableValue.h:43
> +    CSSMutableValue(ClassType classType)

This single-argument constructor should probably be marked explicit.

> Source/WebCore/css/CSSPrimitiveValue.h:225
> +    CSSPrimitiveValue(int ident);
>      CSSPrimitiveValue(unsigned color); // RGB value
>      CSSPrimitiveValue(const Length&);

We should explore putting the explicit keyword on these single-argument constructors.

> Source/WebCore/css/CSSValue.h:135
> +    CSSValue(ClassType classType)
> +        : m_classType(classType)
> +        , m_isPrimitive(isPrimitiveType(classType))
> +        , m_isMutable(isMutableType(classType))
> +        , m_isList(isListType(classType))
> +        , m_isInitial(isInitialType(classType))
> +        , m_isInherited(isInheritedType(classType))

Another way to do this is to include bits to indicate these properties in the constants that are passed in.

> Source/WebCore/css/CSSValue.h:145
> +               || type == ImageClass
> +               || type == CursorImageClass
> +               || type == FontFamilyClass;

These are indented 7 spaces. Instead should be indented 4 spaces.

> Source/WebCore/css/CSSValueList.h:68
> +    CSSValueList(bool isSpaceSeparated);
> +    CSSValueList(CSSParserValueList*);

Should use explicit here.

> Source/WebCore/svg/SVGColor.h:83
> +    SVGColor(const SVGColorType&);

Should mark this explicit.

[Andreas Kling]

Committed r99468: <http://trac.webkit.org/changeset/99468>

[Luke Macpherson]

Hi Andreas, what was the rationale for caching m_isPrimitiveValue and m_isList rather than just calculating from m_classType when needed?

Could we just rearrange the ClassType Enum to allow the inline check to be a single comparison?

[Andreas Kling]

(In reply to comment #7)
> Hi Andreas, what was the rationale for caching m_isPrimitiveValue and m_isList rather than just calculating from m_classType when needed?
> 
> Could we just rearrange the ClassType Enum to allow the inline check to be a single comparison?

Hi Luke,

The idea was to keep isPrimitiveValue() and isValueList() as cheap as possible as a "speculative optimization" since they are used quite a bit.

If you have an idea for removing the need for them without incurring additional compares, I'm all ears :)

[David Barr]

*** Bug 64863 has been marked as a duplicate of this bug. ***