Bug 10827

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.

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.

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

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?

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.

(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. :-)

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.

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.

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).

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.

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!

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