Bug 89248 – [cairo] simplify copyCairoImageSurface

• Status: UNCONFIRMED
• Component: Platform

[arno.]

Hi,

copyCairoImageSurface uses a method to copy a generic surface into another specific surface.
But in copyCairoImageSurface both source and target surfaces are images, so we can take advantage of that, and just copy the data from one surface to the other. That would be more straightforward, and probably more efficient (because cairo_paint does a lot of stuff).

[arno.]

Created attachment 147905 [details]
patch proposal

[arno.]

adding alex since he seems to already have reviewed a few patches in CairoUtilities.cpp

[Martin Robinson]

Comment on attachment 147905 [details]
patch proposal

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

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:185
> +    cairo_format_t format = cairo_image_surface_get_format(originalSurface);
> +    int stride = cairo_image_surface_get_stride(originalSurface);
> +    int width = cairo_image_surface_get_width(originalSurface);
> +    int height = cairo_image_surface_get_height(originalSurface);
> +    unsigned char* data = cairo_image_surface_get_data(originalSurface);
> +
> +    unsigned char* copiedData = static_cast<unsigned char*>(malloc(stride * height));
> +    if (!copiedData)
> +        return 0;
> +
> +    memcpy(copiedData, data, stride * height);
> +
> +    RefPtr<cairo_surface_t> newSurface = adoptRef(cairo_image_surface_create_for_data(copiedData, format, width, height, stride));

Does this change show a measurable performance improvement?

[arno.]

Created attachment 149384 [details]
updated patch

previous patch has a bug: when using cairo_image_surface_create_for_data, pixman does not own the data. We must free it ourself

[arno.]

(In reply to comment #3)
> Does this change show a measurable performance improvement?

http://renevier.net/misc/webkit_89248.html
Currently GtkLauncher, display on average on startup:

10px*10px; 100000 iterations: 450ms
100px*100px; 10000 iterations: 260ms
1000px*1000px; 100 iterations: 180ms

after a few page reloads, times are:

10px*10px; 100000 iterations: 350ms
100px*100px; 10000 iterations: 115ms
1000px*1000px; 100 iterations: 110ms

with the patch, times are:

10px*10px; 100000 iterations: 240ms
100px*100px; 10000 iterations: 215ms
1000px*1000px; 100 iterations: 210ms

and after a few page reloads:

10px*10px; 100000 iterations: 160ms
100px*100px; 10000 iterations: 70ms
1000px*1000px; 100 iterations: 45ms

[arno.]

(In reply to comment #5)
> (In reply to comment #3)
> > Does this change show a measurable performance improvement?
> 
> http://renevier.net/misc/webkit_89248.html

I don't really understand why speed is always faster after a few page reloads but anyway, there is definitively a speed improvement in canvasRenderingContext.createPattern(HTMLCanvasElement)

[Martin Robinson]

(In reply to comment #5)
> (In reply to comment #3)
> > Does this change show a measurable performance improvement?
> 
> http://renevier.net/misc/webkit_89248.html
> Currently GtkLauncher, display on average on startup:
> 
> 10px*10px; 100000 iterations: 450ms
> 100px*100px; 10000 iterations: 260ms
> 1000px*1000px; 100 iterations: 180ms

> with the patch, times are:
> 
> 10px*10px; 100000 iterations: 240ms
> 100px*100px; 10000 iterations: 215ms
> 1000px*1000px; 100 iterations: 210ms
 
So time saved per iteration is:

(450 - 240) / 100000 = 0.0021 ms = 2.1 microseconds
(215 - 115) / 10000 = 0.01 ms = 10 microseconds
(210 - 180) / 100 = 0.3 ms = 300 microseconds

> after a few page reloads, times are:
> 
> 10px*10px; 100000 iterations: 350ms
> 100px*100px; 10000 iterations: 115ms
> 1000px*1000px; 100 iterations: 110ms
> 
> and after a few page reloads:
> 
> 10px*10px; 100000 iterations: 160ms
> 100px*100px; 10000 iterations: 70ms
> 1000px*1000px; 100 iterations: 45ms

(350 - 160) / 100000 = 0.0019 ms = 1.9 microseconds
(115 - 70) / 10000 = 0.0045 ms = 4.5 microseconds
(110 - 45) / 100 = 0.65 ms = 650 microseconds

So it seems that, in the best case, we save just over one half of a millisecond and that's with the creation of a very large pattern. I would feel much better about this kind of micro-optimization if it showed some kind of user-observable speedup. 

There's a benefit to abstraction and using the API of your dependencies. I think that benefit outweighs the benefit of micro-optimization when the optimization has little effect on the user experience.

If this change improves the performance of content in the wild, then perhaps I'll change my tune. It seems there is much more lower hanging fruit though.

[arno.]

(In reply to comment #7)

> So it seems that, in the best case, we save just over one half of a millisecond and that's with the creation of a very large pattern. I would feel much better about this kind of micro-optimization if it showed some kind of user-observable speedup. 

Yes, the performance improvement is really negligible.

> There's a benefit to abstraction and using the API of your dependencies. I think that benefit outweighs the benefit of micro-optimization when the optimization has little effect on the user experience.

The current code use image-surface specific apis either for originalSurface and for newSurface. So, I really hadn't the feeling that my patch were decreasing the abstraction.