Bug 176226 - [GTK][Wayland] Use fast malloc to allocate backing store cairo surfaces data
Summary: [GTK][Wayland] Use fast malloc to allocate backing store cairo surfaces data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2017-09-01 09:10 PDT by Carlos Garcia Campos
Modified: 2017-09-02 01:11 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.88 KB, patch)
2017-09-01 09:23 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-09-01 09:10:17 PDT
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.
Comment 1 Carlos Garcia Campos 2017-09-01 09:23:38 PDT
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.
Comment 2 Build Bot 2017-09-01 09:25:32 PDT
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.
Comment 3 Michael Catanzaro 2017-09-01 09:30:20 PDT
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.
Comment 4 Carlos Garcia Campos 2017-09-01 09:37:31 PDT
(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.
Comment 5 Carlos Garcia Campos 2017-09-02 01:11:04 PDT
Committed r221521: <http://trac.webkit.org/changeset/221521>