std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets
This popped up on the WHATWG spec single-page load, where StyleResolver::CascadedProperties::hasProperty() gets quite hot.
Created attachment 229392 [details] Patch
Comment on attachment 229392 [details] Patch Should we add some assertions that these properties are in-range?
Created attachment 230197 [details] Patch Uploaded another iteration for review, now includes bounds checks in ASSERTs.
(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 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.
Committed r167878: <http://trac.webkit.org/changeset/167878>
(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.