Bug 94048

Summary: SVGPathSegList has O(n) append!
Product: WebKit Reporter: Philip Rogers <pdr>
Component: SVGAssignee: Philip Rogers <pdr>
Status: RESOLVED FIXED    
Severity: Normal CC: enjalot, fmalita, krit, stephen.bannasch, tomhudson, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
First pass
none
Update per reviewer comments none

Description Philip Rogers 2012-08-14 17:03:17 PDT
Original bug: crbug.com/142675

PathSegList has O(n) append! Looks like every time appendItem is called we throw away the whole path and re-create it. This causes us to be really poor at the SVG path list benchmark: http://bl.ocks.org/1296930
Comment 1 Tom Hudson 2012-08-15 05:31:15 PDT
I assume that you're looking at the expandCapacity(size()+1, ...) in appendSlowCase(); doesn't it percolate down to 

reserveCapacity(max(newMinCapacity, max(static_cast<size_t>(16), capacity() + capacity() / 4 + 1)))

which at least tries to grow by 25% each time?
Comment 2 Florin Malita 2012-08-15 07:55:37 PDT
(In reply to comment #1)
> I assume that you're looking at the expandCapacity(size()+1, ...) in appendSlowCase(); doesn't it percolate down to 
> 
> reserveCapacity(max(newMinCapacity, max(static_cast<size_t>(16), capacity() + capacity() / 4 + 1)))
> 
> which at least tries to grow by 25% each time?

It's actually the way dynamic updates to the segment list are handled: SVGPathElement::pathSegListChanged() calls buildSVGPathByteStreamFromSVGPathSegList() which in turn just throws away the current result and rebuilds it -

bool buildSVGPathByteStreamFromSVGPathSegList(const SVGPathSegList& list, SVGPathByteStream* result, PathParsingMode parsingMode)
{
    ASSERT(result);
    result->clear();
    if (list.isEmpty())
        return false;

    SVGPathByteStreamBuilder* builder = globalSVGPathByteStreamBuilder(result);

    OwnPtr<SVGPathSegListSource> source = SVGPathSegListSource::create(list);
    SVGPathParser* parser = globalSVGPathParser(source.get(), builder);
    bool ok = parser->parsePathDataFromSource(parsingMode);

So for each JS path.pathSegList.appendItem() call, the byte stream gets rebuilt from scratch.
Comment 3 Florin Malita 2012-08-15 08:42:36 PDT
BTW, this also means that currently setting the path data in one go (using the 'd' attribute) is a *lot* more efficient, as it avoids the problem above.

I've run a  quick test with a modified benchmark, and using that method CR is roughly twice as fast as FF on my Linux box:


--- bench_orig.html	2012-08-15 11:25:19.521652393 -0400
+++ bench2.html	2012-08-15 11:34:06.524369474 -0400
@@ -84,7 +84,7 @@
         for (i=0; i < num; i++) {
           xpos += (factor * Math.random() - factor_div_2) + (half_width  - xpos) * correction;
           ypos += (factor * Math.random() - factor_div_2) + (half_height - ypos) * correction;
-          path_seglist.appendItem(path.createSVGPathSegLinetoAbs(xpos, ypos));
+	  pdata += ' L ' + xpos + ' ' + ypos;
         }
       };
 
@@ -109,10 +109,12 @@
       var count = 0;
       var elapsed_time = 0;
       var sample_time, sample_start, sample_end;
+      var pdata = 'M ' + half_width + ' ' + half_height;
 
       function runSet() {
         sample_start = +new Date();
         addLineSegments(increment);
+        path.setAttribute('d', pdata);
         sample_end = +new Date();
         sample_time = sample_end-sample_start;
         elapsed_time += sample_time;
Comment 4 Stephen Bannasch 2012-08-15 09:30:00 PDT
The reason I wrote the SVG Path Benchmark http://bl.ocks.org/1296930 the way I did was because my original use case was to show fast dynamically updating lines graphs. 

The extremely slow performance using SVG in webkit-based browsers caused me to recode this feature by overlaying a 2D Canvas over the SVG.

Being able to quickly and efficiently extend and render existing Paths is important to me.
Comment 5 Stephen Bannasch 2012-08-15 09:34:04 PDT
Here's a google spreadsheet that compares performance drawing Paths in both Canvas and SVG on many different browsers and operating systems:

https://docs.google.com/a/concord.org/spreadsheet/ccc?key=0AtvlFoSBUC5kdEZJNVFySG9wSHZka0NsOTZDSkt3Nnc#gid=0
Comment 6 Florin Malita 2012-08-15 09:40:52 PDT
(In reply to comment #4)
> 
> Being able to quickly and efficiently extend and render existing Paths is important to me.

Absolutely, no argument there - this will be addressed. I am just documenting a workaround that others may find useful, not saying that things work as intended :)
Comment 7 Philip Rogers 2012-08-19 22:22:25 PDT
Just a quick update: I put together a demo of a O(1) appendItem() but it's only a hack at the moment as I don't think it's possible to implement the rest of SVGPathSegList's methods (removeItem, replaceItem, or insertItemBefore) in any performant way on our current infrastructure because our internal path representation is a flattened bytestream. I'm debating whether we should go forward with this approach, or restructure how we store paths internally.
Comment 8 Philip Rogers 2012-09-05 13:52:27 PDT
Created attachment 162324 [details]
First pass
Comment 9 Dirk Schulze 2012-09-05 14:30:25 PDT
Comment on attachment 162324 [details]
First pass

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

Needs a real review later. Just some comments and questions.

> Source/WebCore/ChangeLog:9
> +        to&from an SVGPathSegList and a string representation of a path.

to and from   looks better.

> Source/WebCore/ChangeLog:31
> +        * svg/SVGPathByteStream.h:
> +        (WebCore::SVGPathByteStream::append):
> +        * svg/SVGPathElement.cpp:
> +        (WebCore::SVGPathElement::pathSegListChanged):
> +        * svg/SVGPathElement.h:
> +        (SVGPathElement):
> +        * svg/SVGPathParser.cpp:

Some inline comments can be helpful here.

> Source/WebCore/svg/SVGPathByteStream.h:68
> +    void append(SVGPathByteStream* other)
> +    {
> +        for (DataIterator it = other->begin(); it != other->end(); ++it)
> +            append(*it);

Are you adding the other string at the end of the current string? why?

> Source/WebCore/svg/SVGPathElement.cpp:336
> +void SVGPathElement::pathSegListChanged(SVGPathSegRole role, ListModification listModification, unsigned affectedIndex)

usually I am not a fan of adding unused variables. In this case why do we have the unsigned? Second, you can also just omit the name, then you don't need the UNUSED_PARAM makro.

> Source/WebCore/svg/SVGPathElement.cpp:345
> +        if (listModification == ListModificationAppend) {

I see, you want to extend this later.

> Source/WebCore/svg/SVGPathElement.h:96
> +    void pathSegListChanged(SVGPathSegRole, ListModification, unsigned affectedIndex);

As far this is not a virtual function, so why do you introduce the variable if you don't use it? Add it in a later patch when you use it.

> Source/WebCore/svg/SVGPathSegList.cpp:41
> +void SVGPathSegList::commitChange(SVGElement* contextElement, ListModification listModification, unsigned affectedIndex)

Ditto. Remove the index for now.

> LayoutTests/perf/svg-path-appenditem.html:17
> +        path.pathSegList.appendItem(path.createSVGPathSegLinetoAbs(20, 20));

This is what I may not understood above, here you add just a new segment. Above you call the append method that adds another byte stream. Do you create a bytestream just for the one segment?
Comment 10 Philip Rogers 2012-09-05 21:13:15 PDT
Created attachment 162406 [details]
Update per reviewer comments
Comment 11 Philip Rogers 2012-09-05 21:19:53 PDT
(In reply to comment #9)
Thanks for the quick review! I've updated the patch to address your points and made the ChangeLog less bad with a better description and benchmark data.

I've also started a meta-bug about the possibility of not using a byte stream and instead using a different data structure for our internal path representation (if we were to do that, the plumbing added in this patch would still be valid): https://bugs.webkit.org/show_bug.cgi?id=95909.
Comment 12 Nikolas Zimmermann 2012-09-09 23:51:02 PDT
Comment on attachment 162406 [details]
Update per reviewer comments

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

Indeed a new SVGPathByteStream has to be created, serialized from the to-be-appended SVGPathSeg. This is a good idea still, as appending two byte streams is cheaper, than what we do right now (rebuild).
I'd r+ this as-is, but will wait for Dirk, if he has additional comments.

> Source/WebCore/svg/properties/SVGListProperty.h:32
> +    ListModificationUnknown = 0,

I don't think you need to specify = 0,  = 1, .. it's redundant.
Comment 13 Dirk Schulze 2012-09-10 17:09:19 PDT
My concerns are just that it fixes specific cases. We don't have a concept how we want to deal with it on excessive SVG DOM access in general. Especially for insert and other things. Please go ahead with the review, since I am a bit stock with SVG Open at the moment.
Comment 14 Philip Rogers 2012-09-12 16:19:37 PDT
Friendly ping for a review :)
Comment 15 Philip Rogers 2012-09-16 17:27:24 PDT
@Dirk, do you have time to look at this?
Comment 16 Nikolas Zimmermann 2012-09-17 01:47:56 PDT
Comment on attachment 162406 [details]
Update per reviewer comments

Sorry phil, I forgot to login before submitting, two days ago :-( You should have mailed me in private, to remind me.. :-)
Comment 17 WebKit Review Bot 2012-09-17 01:55:42 PDT
Comment on attachment 162406 [details]
Update per reviewer comments

Clearing flags on attachment: 162406

Committed r128729: <http://trac.webkit.org/changeset/128729>
Comment 18 WebKit Review Bot 2012-09-17 01:55:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Stephen Bannasch 2012-09-17 06:47:33 PDT
Thanks a bunch Philip and reviewers!

How can I follow progress for whether, how, and when this change propagates into the the nightly-releases of WebKit-based browsers?

I expect this work to be folded into Chrome and Safari at some point but am not sure what I can track to monitor this progress.

Will this work also end up in webkit-based browsers on other systems?
Comment 20 Philip Rogers 2012-09-17 10:43:09 PDT
Thanks Niko!

(In reply to comment #19)
> Thanks a bunch Philip and reviewers!
> 
> How can I follow progress for whether, how, and when this change propagates into the the nightly-releases of WebKit-based browsers?
> 
> I expect this work to be folded into Chrome and Safari at some point but am not sure what I can track to monitor this progress.
> 
> Will this work also end up in webkit-based browsers on other systems?


The Chrome port releases very often and has 4 channels: canary (aka nightly), dev (chrome 23), beta (chrome 22), and stable (chrome 21). Every couple months the versions tick up one, so stable will be moving to Chrome 22 very soon. This patch is in canary right now if you want to test it, and the patch will likely be in dev channel in the next few days. You can watch the progress here: http://omahaproxy.appspot.com/viewer
base_webkit_revision is the version you're looking for, and this landed in webkit r128729

Safari releases a little slower, I think roughly once or twice a year.

Unfortunately I don't know about the iOS and Android browser release cycle :/
Comment 21 Stephen Bannasch 2012-09-17 11:55:09 PDT
It's in Nightly Safari already.

The total elapsed time drawing 5000 line segments in this benchmark dropped from about 900ms to 9ms!  http://bl.ocks.org/1296930.

I have Chrome 23.0.1269.0 canary installed. It's not reporting any update available and the total elapsed time is still 871 ms.

Presumably the update will appear shortly.
Comment 22 Stephen Bannasch 2012-09-20 21:21:38 PDT
The Nightly version of Safari/Webkit now runs this SVG Path performance benchmark in just 8ms:

http://bl.ocks.org/1296930

While Chrome 24.0.1272.1 canary now takes 18ms.

I've updated my google spreadsheet with these new results:

https://docs.google.com/a/concord.org/spreadsheet/ccc?key=0AtvlFoSBUC5kdEZJNVFySG9wSHZka0NsOTZDSkt3Nnc#gid=0
Comment 23 Stephen Bannasch 2012-09-20 21:39:58 PDT
The just released Nightly Safari/Webkit (5.1.7 (6534.57.2, r129183)) now runs the benchmark in 25ms almost 3 times slower than it did just a day ago. This performance is still reasonably fast and is now somewhat slower than the equivalent Chrome release..
Comment 24 Philip Rogers 2012-09-23 19:10:42 PDT
(In reply to comment #23)
> The just released Nightly Safari/Webkit (5.1.7 (6534.57.2, r129183)) now runs the benchmark in 25ms almost 3 times slower than it did just a day ago. This performance is still reasonably fast and is now somewhat slower than the equivalent Chrome release..

I think it's likely this is an edge case in the V8 <-> SVG code. I've followed this bug to track it:
https://bugs.webkit.org/show_bug.cgi?id=97420

I did confirm that Chromium nightly and Safari nightly have this fix as the time is constant across runs, albeit slightly slower on Chromium.