Bug 131685 – std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets

[Zan Dobersek]

This popped up on the WHATWG spec single-page load, where StyleResolver::CascadedProperties::hasProperty() gets quite hot.

[Zan Dobersek]

Created attachment 229392 [details]
Patch

[Darin Adler]

Comment on attachment 229392 [details]
Patch

Should we add some assertions that these properties are in-range?

[Zan Dobersek]

Created attachment 230197 [details]
Patch

Uploaded another iteration for review, now includes bounds checks in ASSERTs.

[Andreas Kling]

(In reply to comment #1)
> This popped up on the WHATWG spec single-page load, where StyleResolver::CascadedProperties::hasProperty() gets quite hot.

Another way to make hasProperty() less hot would be to make StyleResolver::CascadedProperties a separate list of property ID's that are present. Then StyleResolver::applyCascadedProperties() wouldn't have to iterate over every property ID, checking if each one is present in the set.

[Darin Adler]

Comment on attachment 230197 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:928
> +        ASSERT(!shorthandPropertyID || shortPropertyIndex < shorthandPropertyUsed.size());

Wouldn’t it be better to put this assertion inside the if just below to avoid the "||" here?

> Source/WebCore/css/StyleResolver.cpp:191
> -    bool hasProperty(CSSPropertyID id) const { return m_propertyIsPresent.test(id); }
> +    bool hasProperty(CSSPropertyID id) const
> +    {
> +        ASSERT(id < m_propertyIsPresent.size());
> +        return m_propertyIsPresent[id];
> +    }

Normally when a function body grows to be nontrivial like this we like to put it in the header after the class definition, with the inline keyword, rather than leaving it inside the class definition.

[Carlos Garcia Campos]

Committed r167878: <http://trac.webkit.org/changeset/167878>

[Zan Dobersek]

(In reply to comment #7)
> Committed r167878: <http://trac.webkit.org/changeset/167878>

This actually landed in r167877.
http://trac.webkit.org/changeset/167877

r167878 landed as a follow-up to bug #132228, but the Perl tools messed up Carlos' commit.