Bug 102901 – Implement CSSValue::equals(const CSSValue&) to optimise CSSValue comparison

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexander Pavlov (apavlov)]

This can immensely speed up CSSValue comparisons.

[Alexander Shalamov]

Created attachment 176706 [details]
Patch 1

First patch for initial review.

[WebKit Review Bot]

Attachment 176706 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSCalculationValue.h:64:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSCalculationValue.h:65:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Comment on attachment 176706 [details]
Patch 1

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

A small style sanity check

> Source/WebCore/css/CSSValue.cpp:101
> +        ;    }

inadvertent change?

> Source/WebCore/css/Counter.h:49
> +        && listStyle() == rhs.listStyle()

Continuation lines should be indented relative to the first line

> Source/WebCore/css/DashboardRegion.h:36
> +        && m_isCircle == rhs.m_isCircle && m_isRectangle == rhs.m_isRectangle

Ditto

> Source/WebCore/css/Rect.h:45
> +        && *m_left == *rhs.m_left && *m_bottom == *rhs.m_bottom;

Ditto

[Alexander Shalamov]

Created attachment 176917 [details]
Patch 2

- Removed extra ";" : Source/WebCore/css/CSSValue.cpp:101
- Fixed indentation for: Source/WebCore/css/Counter.h:49, Source/WebCore/css/DashboardRegion.h:36, Source/WebCore/css/Rect.h:45
- Fixed CSSCalcExpressionNode::Type enum to make style checker happy

[Build Bot]

Comment on attachment 176917 [details]
Patch 2

Attachment 176917 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15060635

[Alexander Shalamov]

Created attachment 177233 [details]
Patch 3

- Added #include "CSSPrimitiveValueMappings.h" to CSSValue.cpp

[Build Bot]

Comment on attachment 177233 [details]
Patch 3

Attachment 177233 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15101481

[Darin Adler]

I did not have a chance to read the patch, but I do have a comment: It’s good to implement equality checks. But it’s usually not safe to have a == operator that is virtual, since it’s so easy to slice rather than comparing correctly. I think we probably want a named function rather than the == operator here.

[Alexander Shalamov]

(In reply to comment #8)
> I did not have a chance to read the patch, but I do have a comment: It’s good to implement equality checks. But it’s usually not safe to have a == operator that is virtual, since it’s so easy to slice rather than comparing correctly. I think we probably want a named function rather than the == operator here.

There is no virtual operator in CSSValue, if I would have added virtual method / operator, that would have increased the size of CSSValue and it's children.

I could rename operator== to method bool CSSValue::equals(const CSSValue&) const;

[Alexander Shalamov]

Created attachment 177501 [details]
Patch 4

- rebased to master
- Fix for win bot compilation error. CSSPrimitiveValue conversion operator was generated twice.

[Antti Koivisto]

Comment on attachment 177501 [details]
Patch 4

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

This introduces lots of new code, some of it pretty complex, and I suspect we lack sufficient test coverage for it. Since the operators are already used it should be possible to write a layout test that systematically tests them in all the value types (at minimum verifying that equal values really compare equal).

> Source/WebCore/css/CSSBasicShapes.cpp:81
> +    if (type() == shape.type()) {
> +        const CSSBasicShapeRectangle& rhs = static_cast<const CSSBasicShapeRectangle&>(shape);

It would be nicer to use early returns. 

if (type() != shape.type())
    return false;
...

to avoid indenting large blocks. This comment applies to many places in this patch.

> Source/WebCore/css/CSSBasicShapes.cpp:82
> +        return *m_x == *rhs.m_x && *m_y == *rhs.m_y && *m_width == *rhs.m_width && *m_height == *rhs.m_height

Do we know m_x and pals can't be null? This patch has lots of unexplained dereferencing.

> Source/WebCore/css/CSSBasicShapes.cpp:84
> +            && m_radiusX ? rhs.m_radiusX && *m_radiusX == *rhs.m_radiusX : !rhs.m_radiusX
> +            && m_radiusY ? rhs.m_radiusY && *m_radiusY == *rhs.m_radiusY : !rhs.m_radiusY;

This is fairly hard to read. It might be useful to have an inline template function for comparing CSSValue pointers. 

Some CSSValues, especially CSSPrimitiveValues are shared via CSSValuePool. Equality test should always involve pointer comparison first.

> Source/WebCore/css/CSSBorderImageSliceValue.cpp:53
> +bool CSSBorderImageSliceValue::operator==(const CSSBorderImageSliceValue& rhs) const

I think elsewhere we generally use 'other' as variable name in operator==. 'rsh' sounds strange.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1261
> +    if (m_primitiveUnitType == rhs.m_primitiveUnitType)
> +        switch (m_primitiveUnitType) {

Use early return.

> Source/WebCore/css/CSSValue.cpp:293
> +    if (m_classType == rhs.m_classType) {
> +        switch (m_classType) {

Use early return.

[Antti Koivisto]

> There is no virtual operator in CSSValue, if I would have added virtual method / operator, that would have increased the size of CSSValue and it's children.
> 
> I could rename operator== to method bool CSSValue::equals(const CSSValue&) const;

This is still essentially a virtual method with vtable implemented by hand. I agree with Daring that having this as a named function would be better. CSSValue are always pointers and the required dereferencing is bug prone and looks ugly too.

[Alexander Shalamov]

Created attachment 183132 [details]
Patch 5

Fixed patch according to Antti's comments:

- Renamed operator== to equals method
- Added template function to compare two RefPtrs that contain CSSValue objects
- Used early returns wherever possible
- Added layout test. Test creates stylesheet and then inserts html snippet with inline style that duplicates style in document stylesheet. If inline style and document style css property have equal cssvalues, inline style is removed from inserted element.

[Build Bot]

Comment on attachment 183132 [details]
Patch 5

Attachment 183132 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15942085

[Build Bot]

Comment on attachment 183132 [details]
Patch 5

Attachment 183132 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15914936

[Alexander Shalamov]

Created attachment 183732 [details]
Patch 6

- Fixed dashboard region value comparison.

[Alexander Pavlov (apavlov)]

Comment on attachment 183732 [details]
Patch 6

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

> Source/WebCore/css/CSSBasicShapes.cpp:80
> +    if (type() != shape.type())

For all CSSBasicShapes, you can use the corresponding Type literal instead of type() for the sake of speed, since this->type() value is known at compile time, and there's no need to invoke another virtual method.

> Source/WebCore/css/CSSGradientValue.h:59
> +        && compareCSSValuePtr<CSSPrimitiveValue>(m_position, other.m_position);

missing indent

[Antti Koivisto]

Comment on attachment 183732 [details]
Patch 6

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

r=me with some comments.

> LayoutTests/cssom/cssvalue-comparison-expected.txt:4
> +PASS Two CSSValues "20%" for property "width" are equal. 

This could also include a test per type sanity checking that non-equal values don't compare equal.

> Source/WebCore/css/CSSValue.cpp:81
> +template<class ChildClassType>
> +inline static bool compareCSSValues(const CSSValue& first, const CSSValue& second)
> +{
> +    return static_cast<const ChildClassType&>(first).equals(static_cast<const ChildClassType&>(second));
> +}

This helper could be just above the only function that uses it.

> Source/WebCore/css/CSSValue.h:246
> +    size_t size = firstVector.size();
> +    if (size == secondVector.size()) {
> +        for (size_t i = 0; i < size; i++) {

Could do early return instead.

> Source/WebCore/css/FontValue.cpp:75
> +    return compareCSSValuePtr<CSSPrimitiveValue>(style, other.style)
> +        && compareCSSValuePtr<CSSPrimitiveValue>(variant, other.variant)
> +        && compareCSSValuePtr<CSSPrimitiveValue>(weight, other.weight)
> +        && compareCSSValuePtr<CSSPrimitiveValue>(size, other.size)
> +        && compareCSSValuePtr<CSSPrimitiveValue>(lineHeight, other.lineHeight)
> +        && compareCSSValuePtr<CSSValueList>(family, other.family);

Shouldn't the compiler be able to figure out the template argument type without providing it explicitly pretty much everywhere?

[Ryosuke Niwa]

Excellent! Thanks for the patch. I can make a lot of improvements in the editing code now.

[Alexander Shalamov]

Created attachment 184951 [details]
Patch 7

Fixes based on review comments from Alexander and Antti.
- Used literal to compare CSSBasicShapes type
- Fixed indent in Source/WebCore/css/CSSGradientValue.h:59
- Added test for non-equality of cssvalue objects
- Moved helper function above the function in CSSValue.cpp
- Return early in Source/WebCore/css/CSSValue.h:246
- Omit template arguments whenever possible (hopefully msvc could deduct template parameter from arguments)

[Alexander Pavlov (apavlov)]

Alexander,

Did you mean to set r? on your patch? Otherwise it is not visible in the review queue.

(In reply to comment #20)
> Created an attachment (id=184951) [details]
> Patch 7
> 
> Fixes based on review comments from Alexander and Antti.
> - Used literal to compare CSSBasicShapes type
> - Fixed indent in Source/WebCore/css/CSSGradientValue.h:59
> - Added test for non-equality of cssvalue objects
> - Moved helper function above the function in CSSValue.cpp
> - Return early in Source/WebCore/css/CSSValue.h:246
> - Omit template arguments whenever possible (hopefully msvc could deduct template parameter from arguments)

[WebKit Review Bot]

Comment on attachment 184951 [details]
Patch 7

Rejecting attachment 184951 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 184951, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
SSShaderValue.cpp
patching file Source/WebCore/css/WebKitCSSShaderValue.h
patching file Source/WebCore/css/WebKitCSSTransformValue.h
patching file Source/WebCore/editing/EditingStyle.cpp
patching file Source/WebCore/svg/SVGColor.cpp
patching file Source/WebCore/svg/SVGColor.h
patching file Source/WebCore/svg/SVGPaint.cpp
patching file Source/WebCore/svg/SVGPaint.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16401105

[Alexander Shalamov]

(In reply to comment #21)
> Alexander,
> 
> Did you mean to set r? on your patch? Otherwise it is not visible in the review queue.
> 
> (In reply to comment #20)
> > Created an attachment (id=184951) [details] [details]
> > Patch 7
> > 
> > Fixes based on review comments from Alexander and Antti.
> > - Used literal to compare CSSBasicShapes type
> > - Fixed indent in Source/WebCore/css/CSSGradientValue.h:59
> > - Added test for non-equality of cssvalue objects
> > - Moved helper function above the function in CSSValue.cpp
> > - Return early in Source/WebCore/css/CSSValue.h:246
> > - Omit template arguments whenever possible (hopefully msvc could deduct template parameter from arguments)

I need to rebase it to master.

[Alexander Shalamov]

Created attachment 187521 [details]
Patch 8

Rebased to master
Modified changelog

[WebKit Review Bot]

Comment on attachment 187521 [details]
Patch 8

Clearing flags on attachment: 187521

Committed r142444: <http://trac.webkit.org/changeset/142444>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

Comment on attachment 187521 [details]
Patch 8

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

> Source/WebCore/css/CSSGradientValue.cpp:1170
> +        equalXorY == !other.m_firstX || !other.m_firstY;

This should probably be "=", not "=="? I suppose this branch has no test?

[Darin Adler]

(In reply to comment #27)
> > Source/WebCore/css/CSSGradientValue.cpp:1170
> > +        equalXorY == !other.m_firstX || !other.m_firstY;
> 
> This should probably be "=", not "=="? I suppose this branch has no test?

Good catch. We should make a test.

[Nico Weber]

(In reply to comment #28)
> (In reply to comment #27)
> > > Source/WebCore/css/CSSGradientValue.cpp:1170
> > > +        equalXorY == !other.m_firstX || !other.m_firstY;
> > 
> > This should probably be "=", not "=="? I suppose this branch has no test?
> 
> Good catch. We should make a test.

I have a fix (and test) for this, will upload soon.

[Nico Weber]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > > Source/WebCore/css/CSSGradientValue.cpp:1170
> > > > +        equalXorY == !other.m_firstX || !other.m_firstY;
> > > 
> > > This should probably be "=", not "=="? I suppose this branch has no test?
> > 
> > Good catch. We should make a test.
> 
> I have a fix (and test) for this, will upload soon.

https://bugs.webkit.org/show_bug.cgi?id=122987