Bug 131685 - std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets
Summary: std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-15 11:57 PDT by Zan Dobersek
Modified: 2014-04-28 02:19 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.52 KB, patch)
2014-04-15 12:38 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2014-04-25 14:04 PDT, Zan Dobersek
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-04-15 11:57:00 PDT
std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets
Comment 1 Zan Dobersek 2014-04-15 12:19:54 PDT
This popped up on the WHATWG spec single-page load, where StyleResolver::CascadedProperties::hasProperty() gets quite hot.
Comment 2 Zan Dobersek 2014-04-15 12:38:40 PDT
Created attachment 229392 [details]
Patch
Comment 3 Darin Adler 2014-04-16 10:26:04 PDT
Comment on attachment 229392 [details]
Patch

Should we add some assertions that these properties are in-range?
Comment 4 Zan Dobersek 2014-04-25 14:04:06 PDT
Created attachment 230197 [details]
Patch

Uploaded another iteration for review, now includes bounds checks in ASSERTs.
Comment 5 Andreas Kling 2014-04-27 02:39:59 PDT
(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.
Comment 6 Darin Adler 2014-04-27 19:58:56 PDT
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.
Comment 7 Carlos Garcia Campos 2014-04-28 01:16:50 PDT
Committed r167878: <http://trac.webkit.org/changeset/167878>
Comment 8 Zan Dobersek 2014-04-28 02:19:14 PDT
(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.