Bug 46052 – SVG: Remove "create" methods and use port-specific "add" counterparts

• Component: SVG
• Status: RESOLVED FIXED

[Andreas Kling]

We should use the port-specific Path::addEllipse() to speed up Path::createEllipse() (which is also used by Path::createCircle()) instead of manually constructing the ellipses from lines.

See also: https://lists.webkit.org/pipermail/webkit-dev/2010-September/014419.html

[Andreas Kling]

Created attachment 68030 [details]
Proposed patch (not to be landed before 46051 is resolved)

Note that this patch will require re-baselining of many tests, so I won't be landing it until 46051 is resolved.

[Nikolas Zimmermann]

Comment on attachment 68030 [details]
Proposed patch (not to be landed before 46051 is resolved)

r=me, as soon as the DRT bug is resolved first.

[Andreas Kling]

Created attachment 70238 [details]
Proposed patch v2

New patch as discussed on IRC.
I've removed all the static Path::create* methods and replaced their usage with Path::add*

[Nikolas Zimmermann]

Comment on attachment 70238 [details]
Proposed patch v2

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

Good first try, looking forward to the next version.

> WebCore/platform/graphics/Path.cpp:121
>      if (width <= 0.0f || height <= 0.0f)

Please use if (width <= 0 || height <= 0).
If there are more cases of ".f" postfixes in this file, please kill them.

> WebCore/platform/graphics/Path.cpp:133
>      if (dy > height * 0.5f)
>          dy = height * 0.5f;

s/0.5f/0.5/

> WebCore/platform/graphics/Path.cpp:140
> +    addBezierCurveTo(FloatPoint(x + width - dx * (1 - QUARTER), y), FloatPoint(x + width, y + dy * (1 - QUARTER)), FloatPoint(x + width, y + dy));

QUARTER has an evil name, please rename it to something sensible, and use the "g" prefix.

> WebCore/platform/graphics/Path.cpp:164
>      if (width <= 0.0 || height <= 0.0)

if (width <= 0 || height <= 0)

> WebCore/platform/graphics/Path.cpp:204
> +#if 0

No #if 0 code please remove it alltogether, or is there a difference between addRect and addRectangle?

> WebCore/platform/graphics/Path.h:147
> +        void addRoundedRectangle(const FloatRect&, const FloatSize& roundingRadii);

Can you move them below the existing add methods?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:706
> +    for (unsigned i = 0; i < rectCount; i++) {

While you're at it s/i++/++i/

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1282
> +    {

Why do you wrap this in braces?

> WebCore/rendering/RenderBoxModelObject.cpp:1756
> +                Path path;

Declare the Path path;  outside of hasBorderRadius and...

> WebCore/rendering/RenderBoxModelObject.cpp:1767
> +                Path path;

... reuse it hear (with a path.clear() before)...

> WebCore/rendering/RenderBoxModelObject.cpp:1776
> +                Path path;

...here...

> WebCore/rendering/RenderBoxModelObject.cpp:1780
> +                Path path;

...and here...

> WebCore/svg/SVGCircleElement.cpp:131
> +    path.addEllipse(FloatRect(cx().value(this) - radius,

I wouldn't wrap the lines here.

> WebCore/svg/SVGEllipseElement.cpp:135
> +    float radiusX = rx().value(this);

Micro optimization: immediately check if (radiusX <= 0) return;.
No need to call ry().value() in that case.

> WebCore/svg/SVGEllipseElement.cpp:141
> +    path.addEllipse(FloatRect(cx().value(this) - radiusX,

I wouldn't wrap the lines here.

> WebCore/svg/SVGRectElement.cpp:154
>      FloatRect rect(x().value(this), y().value(this), width().value(this), height().value(this));
>  
> +    if (rect.width() <= 0 || rect.height() <= 0)

This is flawed, please use:
float widthValue = width().value(this);
if (widthValue <= 0)
    return;

float heightValue = height().value(this);
if (heightValue <= 0)
    return;

float xValue = x().value(this);
float yValue = y().value(this);

> WebCore/svg/SVGRectElement.cpp:160
>          float _rx = hasRx ? rx().value(this) : ry().value(this);

if (hasRx || hasRy) {
    float rxValue = rx().value(this);
    float ryValue = ry().value(this);
    if (!hasRx)
        rxValue = ryValue;
    else if (!hasRy)
        ryValue = rxValue;
....
}
This should be sightly faster.

> WebCore/svg/SVGStyledTransformableElement.cpp:110
>  Path SVGStyledTransformableElement::toClipPath() const

toClipPath should also take a Path& parameter.

> WebCore/svg/SVGStyledTransformableElement.h:50
> +    virtual void toPathData(Path&) const {}

Please use { } instead of {}.

[Andreas Kling]

Created attachment 70355 [details]
Proposed patch v4

Updated patch addressing all of Niko's comments.

[Eric Seidel (no email)]

Attachment 70355 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4360008

[Andreas Kling]

Comment on attachment 70355 [details]
Proposed patch v4

Broke MathML, new patch coming..

[Andreas Kling]

Created attachment 70370 [details]
Proposed patch v5

Removed Path::create* usage in MathML code.
Also removed the relevant symbols from WebCore.order

[Eric Seidel (no email)]

Attachment 70370 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4365003

[Andreas Kling]

(In reply to comment #9)
> Attachment 70370 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/4365003

Don't know how to make sense of this log, and it builds fine on a Mac here.

[Nikolas Zimmermann]

Comment on attachment 70370 [details]
Proposed patch v5

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

Almost there, still giving r- as it's not perfect yet.

> WebCore/ChangeLog:5
> +        SVG: Leverage platform's Path::addEllipse() in Path::createEllipse()

The bug title should be changed, you're modifying more than addEllipse.

> WebCore/platform/graphics/Path.cpp:80
> +            static const float rad2deg = 180.0 / piFloat;

Ok, this is unrelated, but please remove thie rad2deg static and use the one in wtf/MathExtras.h

> WebCore/platform/graphics/Path.cpp:128
> +    if (dx > width * 0.5)

I think it makes sense to declare:
float halfWidth = width / 2;
if (dx > halfWidth)
   dx = halfWidth;

As width * 0.5 is used several times below., Same for height * 0.5 Just a little optimization.

> WebCore/platform/graphics/Path.cpp:141
> +    addBezierCurveTo(FloatPoint(x + width - dx * (1 - gCircleControlPoint), y), FloatPoint(x + width, y + dy * (1 - gCircleControlPoint)), FloatPoint(x + width, y + dy));

If we're always using 1 - gCircleControlPoint, that should go into the static directly. so it read FloatPoint(x + width - dx * gCircle...

> WebCore/platform/graphics/Path.cpp:161
> +void Path::addRoundedRectangle(const FloatRect& rectangle, const FloatSize& topLeftRadius, const FloatSize& topRightRadius, const FloatSize& bottomLeftRadius, const FloatSize& bottomRightRadius)

For consistency, this should be renamed addRoundedRect. Or rename addRect to addRectangle.

> WebCore/platform/graphics/Path.cpp:183
> +    addLineTo(FloatPoint(x + width, y + height - bottomRightRadius.height()));

I'd add a newline before this addLineTo line, and split the upper bezier curveTo in two lines,
addBezierCurveTo(FloatPoint(x + width, ..., y),
                                 FloatPoint(x + width, ....),
(aligning the FloatPoints

> WebCore/platform/graphics/Path.cpp:185
> +    addLineTo(FloatPoint(x + bottomLeftRadius.width(), y + height));

Ditto.

> WebCore/platform/graphics/Path.cpp:187
> +    addLineTo(FloatPoint(x, y + topLeftRadius.height()));

Ditto.

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:243
> +    {

Why this extra brace?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:707
> +        Path path;

How about declaring this before the loop, and using if (i > 0) path.clear() here ?

> WebCore/rendering/RenderBoxModelObject.cpp:1766
> +            path.clear();

You only need the clear, if (hasBorderRadius), avoids an unncessary call.

> WebCore/rendering/svg/SVGInlineTextBox.cpp:496
> +    if (fragment.width > 0 && thickness > 0)

I'd change this:
if (fragment.width <= 0 || thickness <= 0)
    return;


And move the float y computation etc. below this check. Early exit if there's nothing to do.

> WebCore/svg/SVGAnimateMotionElement.cpp:118
> +            return path;

Evil Path copying.... but not something you should change now.

[Andreas Kling]

Created attachment 70431 [details]
Proposed patch v6

Updated patch, addressing all of Niko's comments.

[Eric Seidel (no email)]

Attachment 70431 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4328023

[Dirk Schulze]

Comment on attachment 70431 [details]
Proposed patch v6

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

I'm not sure if we should add an ASSERT to all toClipPath as well as toPathData if the path is empty. (not sure how platforms implemented it, but they should check the number of segments internally). Maybe we can trust the programmer to never call this functions with a none empty path ;-)

These are just some notes.

> WebCore/platform/graphics/Path.cpp:112
> +void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize& roundingRadii)

You should use rect instead of rectangle for the variable, at least if you change the function name.

> WebCore/platform/graphics/Path.cpp:117
>      float x = rectangle.x();
>      float y = rectangle.y();
>      float width = rectangle.width();
>      float height = rectangle.height();

Why not FloatRect rect(rectangle) so maybe leave the name above.

> WebCore/platform/graphics/Path.cpp:119
>      float rx = roundingRadii.width();
>      float ry = roundingRadii.height();

Can't we use FloatSize radius here?

If we copy all data, we might just use
void Path::addRoundedRect(FloatRect rect, FloatSize roundingRadii) instead of void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize& roundingRadii). What do you think?

> WebCore/platform/graphics/Path.cpp:126
> +    float dx = rx;
> +    float dy = ry;
> +    float halfWidth = width / 2;
> +    float halfHeight = height / 2;

Maybe we can use FloatSize for the 4 values as well.

[Dirk Schulze]

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

[Andreas Kling]

Created attachment 70451 [details]
Proposed patch v7

Addressed Dirk's comments. Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds.

[Darin Adler]

(In reply to comment #16)
> Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds.

Normally we include assertions even if they would slow down debug builds. Is the slowdown so dramatic it causes a major problem?

[Dirk Schulze]

Comment on attachment 70451 [details]
Proposed patch v7

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

> WebCore/ChangeLog:9
> +        Circles and ellipses will be significantly faster on platforms that
> +        implement Path::addEllipse() in a sane fashion.

Can you mention, that you avoid copying paths?

> WebCore/platform/graphics/Path.cpp:118
> +    float dx = roundingRadii.width();
> +    float dy = roundingRadii.height();

Why not FloatSize radius= roundingRadii; ?

> WebCore/platform/graphics/Path.cpp:120
> +    float halfWidth = rect.width() / 2;
> +    float halfHeight = rect.height() / 2;

You could use FloatSize here as well. But it is also ok to use two floats.

[Nikolas Zimmermann]

(In reply to comment #17)
> (In reply to comment #16)
> > Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds.
> 
> Normally we include assertions even if they would slow down debug builds. Is the slowdown so dramatic it causes a major problem?

We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past.

But as it's an assertion, that's not present in release builds, maybe it's not so dramatic after all.

[Darin Adler]

(In reply to comment #19)
> We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past.

Note that if CGPathIsEmpty was showing up as a bottleneck in a way that’s difficult to avoid, we could possibly change the augment CG implementation of Path to cache additional state so that in most cases a call to CGPathIsEmpty would not be necessary.

[Eric Seidel (no email)]

Attachment 70451 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4368010

[Nikolas Zimmermann]

(In reply to comment #20)
> (In reply to comment #19)
> > We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past.
> 
> Note that if CGPathIsEmpty was showing up as a bottleneck in a way that’s difficult to avoid, we could possibly change the augment CG implementation of Path to cache additional state so that in most cases a call to CGPathIsEmpty would not be necessary.

That's right, a simple boolean would be sufficient. Let's add the assertions and see if it's still a problem, if so, we'll just cache a "bool m_isEmpty" variable in PathCg.

[Nikolas Zimmermann]

Comment on attachment 70451 [details]
Proposed patch v7

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

Please add the path isEmpty assertion in toPathData, an let's find out why it doesn't build on mac...

> WebCore/platform/graphics/Path.cpp:132
> +    moveTo(FloatPoint(rect.x() + dx, rect.y()));

Please cache rect.x() / y() / width() / height() in local variables.

> WebCore/platform/graphics/Path.cpp:162
> +    if (rect.width() < topLeftRadius.width() + topRightRadius.width()

Please cache rect.x() / y() / width() / height() in local variables.

[Nikolas Zimmermann]

(In reply to comment #23)
> (From update of attachment 70451 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70451&action=review
> 
> Please add the path isEmpty assertion in toPathData, an let's find out why it doesn't build on mac...
> 
> > WebCore/platform/graphics/Path.cpp:132
> > +    moveTo(FloatPoint(rect.x() + dx, rect.y()));
> 
> Please cache rect.x() / y() / width() / height() in local variables.
> 
> > WebCore/platform/graphics/Path.cpp:162
> > +    if (rect.width() < topLeftRadius.width() + topRightRadius.width()
> 
> Please cache rect.x() / y() / width() / height() in local variables.

I take those back, as rect.x() etc. are inline function calls.

[Andreas Kling]

Created attachment 70460 [details]
Proposed patch v8

Added the ASSERTs.
Didn't cache x(), y() etc in locals though since they're inline functions (as discussed on IRC.)

[Dirk Schulze]

Comment on attachment 70460 [details]
Proposed patch v8

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

> WebCore/platform/graphics/Path.cpp:117
> +    FloatSize radius = roundingRadii;

Use FloatSize radius(roundingRadii);

> WebCore/svg/SVGStyledTransformableElement.cpp:118
> +    ASSERT(path.isEmpty());
> +
> +    toPathData(path);

You're calling ASSERT(path.isEmpty()); in toPathData, no need to assert twice.

Please change this before landing. Great patch! r=me.

[Andreas Kling]

Committed r69517: <http://trac.webkit.org/changeset/69517>

[Eric Seidel (no email)]

Attachment 70460 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4275026

[Andreas Kling]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/69517 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #30)
> http://trac.webkit.org/changeset/69517 might have broken Qt Linux Release

Qt specifix expected file updated in http://trac.webkit.org/changeset/69565