SVGPathElement should only build SVGPathSeg* list on demand

• Component: SVG
• Status: RESOLVED DUPLICATE

[Eric Seidel (no email)]

SVGPathElement should only build SVGPathSeg* tree on demand

Right now SVGPathElement, builds the SVGPathSeg tree while parsing.  Then using the toPathData() method, it builds a WebCore::Path object from that tree (which is used by the renderer).

The SVGPathSeg intermediaries are completely unnecessary however.  They're only needed for the DOM interfaces (and possibly for animation).

I would propose that instead the String representation of the path (has held by the attributes dictionary) is the authoritative version, and that toPathData() build the WebCore::path directly from the string.  The SVGPathSeg list could be later built from the WebCore::path or the String as needed.  the WebCore::Path need not be built until the SVGPathElement is actually attached (a renderer is created).  And the SVGPathSeg list need not be built until the relevant DOM properties are accessed.

This would result in a very large speed increase for SVG rendering, as it would get rid of thousands of mallocs associated with building the SVGPathSeg list.

[Eric Seidel (no email)]

We spend well over 15% of our time while parsing large SVGs in malloc/free.  I expect much of that is caused by this one bug.

[Rob Buis]

Created attachment 33051 [details]
First attempt

This patch actually touches multiple bugs, but mainly it allows normalized pathseg lists and tries to build them lazily. I already added some tests
to test synchronization of both lists, which seems to work ok. The patch is still inefficient in some places but I thought it may be good to show it
early for feedback. What needs to be tested more is things like:

var seg = path.pathSegList.getItem(1);
path.setElement('d', 'some other path data')

now what should seg be like?
Also:
var seglist = path.pathSegList;
path.setElement('d', 'some other path data')

is seglist then the same as path.pathSegList?
It may be that the patch needs to be changed anyway to match the behaviour of such cases.
Cheers,

Rob.

[Nikolas Zimmermann]

Rob, this patch looks very interessting - do you plan to finish this?

[Dirk Schulze]

I would also like to see progress on this bug. Rob, do you plan to update this patch to trunk? I like the general rebuildPathInfo() methot, but why don't you hand the content of the dAttr over to it? I guess m_ignoreAttributeChanges is intended as an interupt. Can you give some further comments on it?
What is missing on the normalizedPathSegList-side?

[Rob Buis]

Hello Dirk,

(In reply to comment #4)
> I would also like to see progress on this bug. Rob, do you plan to update this patch to trunk? I like the general rebuildPathInfo() methot, but why don't you hand the content of the dAttr over to it? I guess m_ignoreAttributeChanges is intended as an interupt. Can you give some further comments on it?
> What is missing on the normalizedPathSegList-side?

Unfortunately I am more time pressed this month as usual, so I wont touch this patch for a while. If you want I can comment on the patch this weekend (I need to reinterpret it as well since I didnt look at it for over a year). If you want to work on it, no problem :)
Cheers,

Rob.

[Dirk Schulze]

(In reply to comment #5)
> Hello Dirk,
> 
> (In reply to comment #4)
> > I would also like to see progress on this bug. Rob, do you plan to update this patch to trunk? I like the general rebuildPathInfo() methot, but why don't you hand the content of the dAttr over to it? I guess m_ignoreAttributeChanges is intended as an interupt. Can you give some further comments on it?
> > What is missing on the normalizedPathSegList-side?
> 
> Unfortunately I am more time pressed this month as usual, so I wont touch this patch for a while. If you want I can comment on the patch this weekend (I need to reinterpret it as well since I didnt look at it for over a year). If you want to work on it, no problem :)
> Cheers,
> 
> Rob.

Hi Rob, would be realy great if you can add further comments. Thank you for taking the time. :-)

[Rob Buis]

My answers below...

(In reply to comment #4)
 I like the general rebuildPathInfo() methot, but why don't you hand the content of the dAttr over to it?

That is indeed at least in once case more efficient, in the other case the attribute value still must
be fetched. But indeed it is more efficient overall.

 I guess m_ignoreAttributeChanges is intended as an interupt. Can you give some further comments on it?

In the patch it is needed since otherwise svgAttributeChanged -> rebuildPathInfo -> svgAttributeChanged ec. recursion. Seeing that SVGPolyElement removed a similar construction,
it is possible things are handled differently right now and it is not needed anymore.

> What is missing on the normalizedPathSegList-side?

Nothing I think, but I posed some possible problematic cases or cases where I didnt know what to do above.

I think what needs to be done with the patch is, first, to fix it against the bitrot, ie. make it compile and work as before. Then rebuildPathInfo could be changed as discussed. Then it could be tried to remove m_ignoreAttributeChanges. Finally the problematic cases should be analyzed/fixed.
Cheers,

Rob.

[Dirk Schulze]

I updated the patch of Rob to work on trunk. The patch does not fix all bugs on the block list and if I get it right, changing the d-Attribute by DOM is also not correctly handled yet. I'll work on the remaining issues.

[Dirk Schulze]

We might fix bug 27279 first before fixing this bug for speed reasons. I think we shouldn't create the pathSegList's for every path. It's maybe better, to parse the string in dAttr and create a Path and somthing like a vector for later pathSegList creation simultaneously. This way we don't have a performance regression for the normal painting without accessing the SVGDOM (the regular case).

[Dirk Schulze]

Created attachment 67164 [details]
First proposal, not for landing

This is the first proposal. Use byteStream as the basic storage. dAttr and bot segLists are build on demand.

[Nikolas Zimmermann]

Comment on attachment 67164 [details]
First proposal, not for landing

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

> WebCore/bindings/scripts/CodeGeneratorJS.pm:332
> +    # SVGPathSegments and SVGPathSegmentLists don't need a context parameter. SVGPathElement manages changes.
SVGPathSeg and SVGPathSegList - sounds less confusing.

> WebCore/svg/SVGAnimatedPathData.h:36
>          virtual SVGPathSegList* animatedPathSegList() const = 0;
For consistency, I'd also deconstify these methods, even if they are not implemented so far.

> WebCore/svg/SVGPathElement.cpp:52
> +    m_pathByteStream = SVGPathByteStream::create();
Move this to the initialization list.

> WebCore/svg/SVGPathElement.cpp:179
> +        if (m_isUpdatingPathData)
I'd move it to the top of parseMappedAttribute.

> WebCore/svg/SVGPathElement.cpp:210
> +        RefPtr<Attr> attr = getAttributeNode(SVGNames::dAttr.localName());
Why don't you just use getAttribute(SVGNames::dAttr) here?

> WebCore/svg/SVGPathElement.cpp:212
> +        updatePathSegListFromByteStream();
Isn't it possible to delay the pathSegList update, until someone access us? I guess not, as the seg lists could be stored somewhere in a JS var.

> WebCore/svg/SVGPathElement.cpp:213
> +        updateNormalizedPathSegListFromByteStream();
Ditto. But I guess you have no option, but doing a sync update here, right?

> WebCore/svg/SVGPathElement.cpp:244
> +        RefPtr<Attr> attr = getAttributeNode(SVGNames::dAttr.localName());
This value is not used at all.

> WebCore/svg/SVGPathElement.cpp:262
> +{
ASSERT(m_pathByteStream) and ASSERT(pathSegList) here.

> WebCore/svg/SVGPathElement.cpp:266
> +    if (pathSegList->segListType() == unalteredSegList) {
Is the segListType() concept really needed, if you could just directly compare wheter pathSegList == m_pathSegList, or wheter pathSegList == m_normalizedPathSegList?

> WebCore/svg/SVGPathElement.cpp:288
> +    m_pathSegList->clear(ec);
You should ASSERT(!ec) afterwards.

> WebCore/svg/SVGPathElement.cpp:299
> +    m_normalizedPathSegList->clear(ec);
You should ASSERT(!ec) afterwards.

> WebCore/svg/SVGPathElement.cpp:308
> +        ASSERT(m_pathSegList);
This assertion is bogus, if you just created the list it's not needed.

> WebCore/svg/SVGPathElement.cpp:321
> +        ASSERT(m_normalizedPathSegList);
The assertion is bogus, if you just created the list it's not needed.

> WebCore/svg/SVGPathElement.h:115
>          mutable RefPtr<SVGPathSegList> m_pathSegList;
> +        mutable RefPtr<SVGPathSegList> m_normalizedPathSegList;
Why are those still mutable? You've removed the const from pathSegList/normalizedPathSegList, so it's superfluous now.

> WebCore/svg/SVGPathParserFactory.cpp:90
> +static SVGPathTraversalStateBuilder* globalSVGPathTraversalStateBuilder(PathTraversalState* traversalState, float length)
Hm, this method is already existant, just with a "PathTraversalState& traversalState" parameter. Just reuse it and remove this one.

> WebCore/svg/SVGPathParserFactory.cpp:267
> +    OwnPtr<SVGPathByteStream> stream = SVGPathByteStream::create();
You can directly write "result = SVGPathByteStream::create()" here. No need to store in a local variable first, and then use "result = stream.release()" some lines below.
This also affects buildSVGPathByteStreamFromString / buildAnimatedSVGPathByteStream. Can you fix those as well?

> WebCore/svg/SVGPathParserFactory.cpp:316
> +    if (stream->isEmpty())
ASSERT(stream) before using it.

> WebCore/svg/SVGPathParserFactory.h:52
>      bool getSVGPathSegAtLengthFromSVGPathSegList(SVGPathSegList*, float, unsigned long&);
This seems redundant now, nobody uses it, just remove it.

> WebCore/svg/SVGPathSeg.h:93
> +    SVGPathSegList* m_pathSegList;
You need to initialize this variable in the constructor.

> WebCore/svg/SVGPathSegArc.h:48
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:55
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:60
> +       		m_r1 = r1;
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:69
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:76
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:83
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegArc.h:90
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:39
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:46
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:53
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:60
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:67
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubic.h:74
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubicSmooth.h:45
> +    	}
Wrong indentation.

> WebCore/svg/SVGPathSegCurvetoCubicSmooth.h:52
> +    	}
Wrong indentation.... everywhere in al SVGPathSeg* files, I'm stopping here.

> WebCore/svg/SVGPathSegList.h:32
> +    unalteredSegList,
> +    normalizedSegList
These names are awful :-)
"UnalteredSegmentList"
"NormalizedSegmentList"
sounds better.

Overall, a wonderful patch!

[Nikolas Zimmermann]

*** This bug has been marked as a duplicate of bug 49580 ***