Bug 176226 – [GTK][Wayland] Use fast malloc to allocate backing store cairo surfaces data

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It reduces the memory consumption and improves the performance. Also, since performance is better we don't need to keep the scroll surface allocated all the time, we can create it on demand and keep it allocated only while scrolling too fast.

[Carlos Garcia Campos]

Created attachment 319601 [details]
Patch

This reduced my ephy UI process memory from 307MB to 183MB. Still higher than using X11 (54MB), but much better.

[Build Bot]

Attachment 319601 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:37:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:44:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 319601 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:37
> +    cairo_surface_set_user_data(surface.get(), &s_surfaceDataKey, surfaceData, [](void* data) { fastFree(data); });

Style checker doesn't like this, and I agree that it would be easier to read on two lines. But up to you.

> Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:44
> +    , m_scrolledHysteresis([this](HysteresisState state) { if (state == HysteresisState::Stopped) m_scrollSurface = nullptr; }, scrollHysteresisDuration)

Style checker doesn't like this either. Please let's not use lambdas in initializer lists. Can't you initialize it inside the body of the constructor?

> Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.h:21
>  #include "BackingStoreBackendCairo.h"

I think it belongs inside the #if USE(CAIRO) guard.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 319601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319601&action=review

Thanks for the review.

> > Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:37
> > +    cairo_surface_set_user_data(surface.get(), &s_surfaceDataKey, surfaceData, [](void* data) { fastFree(data); });
> 
> Style checker doesn't like this, and I agree that it would be easier to read
> on two lines. But up to you.

I agree when using a longer body, but in this case I think it reads better this way.

> > Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.cpp:44
> > +    , m_scrolledHysteresis([this](HysteresisState state) { if (state == HysteresisState::Stopped) m_scrollSurface = nullptr; }, scrollHysteresisDuration)
> 
> Style checker doesn't like this either. Please let's not use lambdas in
> initializer lists. Can't you initialize it inside the body of the
> constructor?

No, because it's stack allocated.

> > Source/WebCore/platform/graphics/cairo/BackingStoreBackendCairoImpl.h:21
> >  #include "BackingStoreBackendCairo.h"
> 
> I think it belongs inside the #if USE(CAIRO) guard.

There's no problem because BackingStoreBackendCairo.h is already guarded, but I agree it's better inside the guard here in the header.

[Carlos Garcia Campos]

Committed r221521: <http://trac.webkit.org/changeset/221521>