Bug 71352 – [EFL] Port tiled backing store

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Tomasz Morawski]

Ported tiled backing store to EFL WebKit port.

[Tomasz Morawski]

Created attachment 113302 [details]
Initial implementation

Initial patch for discussion

[Grzegorz Czajkowski]

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

> Source/WebKit/efl/ewk/ewk_view.cpp:2286
> +#if USE(TILED_BACKING_STORE)

Please add an internal documentation for newly added functions.

> Source/WebKit/efl/ewk/ewk_view.cpp:2287
> +Eina_Bool ewk_view_setting_tiled_backingstore_enable_set(Evas_Object* o, Eina_Bool enable)

I would rather prefer using a new WebKit coding style, ewkView instaed of o, smartData instead of sd etc.

> Source/WebKit/efl/ewk/ewk_view.cpp:2650
> +void ewk_view_backingstore_invalidate(Evas_Object* o, const Eina_Rectangle *area)

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:2663
> +void ewk_view_backingstore_adjuct_visible_rect(Evas_Object* o)

Ditto.

[Gyuyoung Kim]

Comment on attachment 113302 [details]
Initial implementation

Attachment 113302 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10258254

[Tomasz Morawski]

(In reply to comment #3)
> (From update of attachment 113302 [details])
> Attachment 113302 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/10258254
Compilation error due to that this patch depends on 71350. 

Anyway, please also check if EFL building bot has libcairo-1.10.2 or more recent.

[KwangHyuk]

> Source/WebKit/efl/ewk/ewk_private.h:162
> +EAPI void ewk_view_backingstore_invalidate(Evas_Object* o, const Eina_Rectangle *area);

Would you check the location of '*' for parameter area ? and I prefer ewkView instead of one character 'o'. :)
In addition, backing_store looks more natural.

> Source/WebKit/efl/ewk/ewk_private.h:163
> +EAPI void ewk_view_backingstore_adjuct_visible_rect(Evas_Object* o);

May adjuct mean adjust ? and  prefer ewkView instead of one character 'o'.
In addition, Would you check the sequence of each word for API ?

> Source/WebKit/efl/ewk/ewk_private.h:164
> +EAPI Eina_Bool ewk_view_setting_tiled_backingstore_enable_set(Evas_Object* o, Eina_Bool enable);

As ewk_private.h is for internal usage, I think that EAPI won't be required in here.

[Gyuyoung Kim]

Comment on attachment 113302 [details]
Initial implementation

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

> Source/WebCore/CMakeListsEfl.txt:109
> +        platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp

This patch doesn't include "TiledBackingStoreBackendCairo.cpp" file. You need to add this file or remove this from CMakeListsEfl.txt.

[Tomasz Morawski]

(In reply to comment #6)
> (From update of attachment 113302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113302&action=review
> 
> > Source/WebCore/CMakeListsEfl.txt:109
> > +        platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp
> 
> This patch doesn't include "TiledBackingStoreBackendCairo.cpp" file. You need to add this file or remove this from CMakeListsEfl.txt.
Could you please tell me why? This patch depends on patch 71350 which added few new files. This means that missing files will revel when the 71350 patch will merged. The "depends on" also means that this patch can not be merged and builded without 71350. So, I don't think that I should remove anythink from CMakeListEfl.txt file.

[Tomasz Morawski]

Created attachment 113631 [details]
Updated according to Grzegorz's suggestions

[Gyuyoung Kim]

Comment on attachment 113631 [details]
Updated according to Grzegorz's suggestions

Attachment 113631 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10318069

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 113631 [details]
Updated according to Grzegorz's suggestions

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

I'd use "backing_store" instead of "backingstore" in the function names as they are separate words.

I'd also like to know how this is going to interact with the tiled backing store implementation in ewk (the API added to ewk_view looks weird if one is using it instead of the single backing store, to start with).

> ChangeLog:3
> +        [EFL] Port tiled backing store

This sounds a bit misleading: the EFL port already has a tiled backing store; I'd rephrase it as "Make it possible to use WebCore's tiled backing store in EFL" or something like that.

> Source/WebCore/CMakeListsEfl.txt:106
> +  IF (WTF_USE_TILED_BACKING_STORE)

Isn't it always on?

> Source/WebCore/CMakeListsEfl.txt:150
> +IF (WTF_USE_TILED_BACKING_STORE)

Ditto.

> Source/WebKit/efl/ChangeLog:8
> +        Ported tiled backing store to EFL WebKit port.

Same thing about the misleading phrase.

> Source/WebKit/efl/ChangeLog:14
> +        Major changes:
> +        - Added new functions to ChromeClientEfl
> +        - Added new functions to ewk_view
> +        - From ewk_view_single calls
> +          ewk_view_setting_tiled_backingstore_enable_set to enable/disable
> +          use of tiled backing store

This is already evident from the lines below; it'd be good to know the motivation and how this interacts with the tiled backing store implementation in ewk.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:67
> +#if USE(TILED_BACKING_STORE)

Isn't it always on?

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:441
> +    WebCore::FloatRect rect = ewk_view_page_rect_get(m_view);
> +    Evas_Object* frame = ewk_view_frame_main_get(m_view);

These can be const.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:516
> +    Eina_Rectangle area;
> +    EINA_RECTANGLE_SET(&area, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height());
> +
> +    ewk_view_backingstore_invalidate(m_view, &area);

This is a private function, you could just pass the IntRect to tehe function.

[Tomasz Morawski]

Created attachment 114005 [details]
Updated according to suggestions

[Gyuyoung Kim]

Comment on attachment 114005 [details]
Updated according to suggestions 

Attachment 114005 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10375021

[Tomasz Morawski]

> I'd also like to know how this is going to interact with the tiled backing store implementation in ewk (the API added to ewk_view looks weird if one is using it instead of the single backing store, to start with).
Hi, 
Thank you for your review. I have uploaded a new version of patch. 
No, this patch contains WebCore's tiled backing store implementation. This implementation should not interact with the old ewk implementation. I agree with you according to API names. They are not good. Do you have any proposition of API name changes to not mix backing stores implementations? Do you think that it is good idea to enabled this feature for ewk_view_single as it is currently done? I would like to discuss this. 

>> +#if USE(TILED_BACKING_STORE)
> Isn't it always on?
No, this flag is defined in CMakeList file so, it could be disabled.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 114005 [details]
Updated according to suggestions 

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

> Source/WebKit/efl/ewk/ewk_private.h:165
> +void ewk_view_backing_store_invalidate(Evas_Object* o, const WebCore::IntRect& area);
> +void ewk_view_backing_store_adjuct_visible_rect(Evas_Object* o);
> +Eina_Bool ewk_view_setting_tiled_backing_store_enable_set(Evas_Object* o, Eina_Bool enable);

I think you can omit "o" as a parameter name, and the Eina_Bool can be a simple bool.

> Source/WebKit/efl/ewk/ewk_view.cpp:2288
> +#if USE(TILED_BACKING_STORE)

I think these #if checks should be moved to the function bodies, so the function symbols are always present.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:56
> +    // use WebCore's backing store

Unneeded.

> Source/cmake/OptionsEfl.cmake:52
> +SET(WTF_USE_TILED_BACKING_STORE 1)
> +ADD_DEFINITIONS(-DWTF_USE_TILED_BACKING_STORE=1)

Thinking about this again, wouldn't it be better to make it an actual option? This way if one uses the build-webkit script the --tiled-backing-store can have an effect. It would also make the #if USE(TILED_BACKING_STORE) checks make sense, as right now they check for something that is always on.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13) 
> Do you think that it is good idea to enabled this feature for ewk_view_single as it is currently done? I would like to discuss this.

It all boils down to whether ewk's tiled backing store still works as expected and if there are any gains to enabling this in the single backing store. Running the layout tests would be very helpful to make sure there are no regressions related to the patch.

> >> +#if USE(TILED_BACKING_STORE)
> > Isn't it always on?
> No, this flag is defined in CMakeList file so, it could be disabled.

You set it with SET(...) and then you call ADD_DEFINITION(...). The only way this can be disabled is if one manually edits OptionsEfl.cmake; if one does so, it's not really our problem.

[Tomasz Morawski]

(In reply to comment #14)
> (From update of attachment 114005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114005&action=review
> 
> > Source/WebKit/efl/ewk/ewk_private.h:165
> > +void ewk_view_backing_store_invalidate(Evas_Object* o, const WebCore::IntRect& area);
> > +void ewk_view_backing_store_adjuct_visible_rect(Evas_Object* o);
> > +Eina_Bool ewk_view_setting_tiled_backing_store_enable_set(Evas_Object* o, Eina_Bool enable);
> 
> I think you can omit "o" as a parameter name, and the Eina_Bool can be a simple bool.
OK, I will change that. Anyway, removing only "o" parameter will look strange for me. Maybe I will raname "o" to "ewkView"? 

> > Source/WebKit/efl/ewk/ewk_view.cpp:2288
> > +#if USE(TILED_BACKING_STORE)
> 
> I think these #if checks should be moved to the function bodies, so the function symbols are always present.
Due to that the Tiled Backing Store support has to be compiled into the WebCore and these functions are not in public API. I think there is no need to move these #if checks to functions body. 

> > Source/cmake/OptionsEfl.cmake:52
> > +SET(WTF_USE_TILED_BACKING_STORE 1)
> > +ADD_DEFINITIONS(-DWTF_USE_TILED_BACKING_STORE=1)
> 
> Thinking about this again, wouldn't it be better to make it an actual option? This way if one uses the build-webkit script the --tiled-backing-store can have an effect. It would also make the #if USE(TILED_BACKING_STORE) checks make sense, as right now they check for something that is always on.
Yes, it can be done. 

> >> +#if USE(TILED_BACKING_STORE)
> > Isn't it always on?
> No, this flag is defined in CMakeList file so, it could be disabled.
You set it with SET(...) and then you call ADD_DEFINITION(...). The only way this can be disabled is if one manually edits OptionsEfl.cmake; if one does so, it's not really our problem.

I have make this like cairo option:
SET(WTF_USE_CAIRO 1)
ADD_DEFINITIONS(-DWTF_USE_CAIRO=1)

[Tomasz Morawski]

Created attachment 115339 [details]
Updated

Updated according to suggestions. Added additional description to ChangeLogs.

[Raphael Kubo da Costa (:rakuco)]

The code looks OK (you might also want to change the default option value in Tools/Scripts/build-webkit too).

However, I'm still waiting for an answer for the other part of my comment #15: are there performance gains with this feature? Have the layout tests been run so there are no regressions?

[Tomasz Morawski]

(In reply to comment #18)
> The code looks OK (you might also want to change the default option value in Tools/Scripts/build-webkit too).
I will change this.
 
> However, I'm still waiting for an answer for the other part of my comment #15: are there performance gains with this feature? Have the layout tests been run so there are no regressions?
I am sorry for lack of response for my side. 

Currently I don't have any performance test done yet. So, I can give you any answer for your question now. I have plan to do such tests till end of December. Would you like to get result of such tests?

No, there was no layout test run for this. This patch and 71350 patch doesn't change any code related to layout mechanism. There was not change in backing store class that fully controls all invalidation and drawing functionality.
It is a good idea to do this. Thank you. I will run layout tests and put result on this bugs.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> Currently I don't have any performance test done yet. So, I can give you any answer for your question now. I have plan to do such tests till end of December. Would you like to get result of such tests?

It'd be good if you could paste them or attach them to this report. I just have something like "I am proposing this change because it improves X and Y by Z%" in mind.

[Tomasz Morawski]

(In reply to comment #20)
> (In reply to comment #19)
> > Currently I don't have any performance test done yet. So, I can give you any answer for your question now. I have plan to do such tests till end of December. Would you like to get result of such tests?
> 
> It'd be good if you could paste them or attach them to this report. I just have something like "I am proposing this change because it improves X and Y by Z%" in mind.

Yes, I understand that it could be a good. But I have not written this patch to prove that ewk's tbs is worse or better then WebCore's tbs. Anyway, what if the webcore's tbs will be worse? I can not write "I am proposing this change because it decrease X and Y by Z%" :) Should we reject this patch in that situation? I have only written this patch to get support of WebCore's backing store and allows to use the genernic WebCore's backing store in the EFL port. I thnik the end user of WebKit's EFL port should decided what backing store should use if any.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #21)
> Yes, I understand that it could be a good. But I have not written this patch to prove that ewk's tbs is worse or better then WebCore's tbs. Anyway, what if the webcore's tbs will be worse? I can not write "I am proposing this change because it decrease X and Y by Z%" :) Should we reject this patch in that situation? I have only written this patch to get support of WebCore's backing store and allows to use the genernic WebCore's backing store in the EFL port. I thnik the end user of WebKit's EFL port should decided what backing store should use if any.

I see. Now that the latest patch version allows one to enable and disable WebCore's tiled backing store at will this is indeed true.

As for the build-webkit change, it might be better to leave it off by default until it can be shown that it performs better than the current implementation, IMO.

[Tomasz Morawski]

> As for the build-webkit change, it might be better to leave it off by default until it can be shown that it performs better than the current implementation, IMO.
Yes, it could be a good solution. Moreover, it will allow to continue work on WebCore's tbs with use of our EFL port in more convenient way. I have only two concerns about this. I don't even know if this patch can be compiled on EFL buildbot due to lack of knowelage about libcairo version installed on it. And the second one our port could has unnoticed build break if somebody change WebCore's tbs client API (due to the EFL buildbot will not compile disabled code).

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #23)
> I have only two concerns about this.

> I don't even know if this patch can be compiled on EFL buildbot due to lack of knowelage about libcairo version installed on it.

Does it require a very recent cairo version to work? Does it fail in older ones because it needs newer API or due to rendering failures?

We can ask the people who take care of the buildbot about that. Gyuyoung, are you there?

> And the second one our port could has unnoticed build break if somebody change WebCore's tbs client API (due to the EFL buildbot will not compile disabled code).

That's true to any code path that depends on features which are disabled by default. Until we think of a better way to deal with this, I can only recommend updating your checkout frequently and making sure the code still builds.

[Tomasz Morawski]

(In reply to comment #24)
> (In reply to comment #23)
> > I have only two concerns about this.
> 
> > I don't even know if this patch can be compiled on EFL buildbot due to lack of knowelage about libcairo version installed on it.
> 
> Does it require a very recent cairo version to work? Does it fail in older ones because it needs newer API or due to rendering failures?
No, no fail. The older version then 1.10 doesn't contain cairo_region_* API that is used in TileCairo. I have used recent version 1.10.2 that is almost one year old. 

Thank you.

[Raphael Kubo da Costa (:rakuco)]

AFAIR we raised the minimum version we required to 1.10 some time ago due to the cairo_region API, so there's no need to worry about that.

[KwangHyuk]

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

> Source/WebKit/efl/ewk/ewk_private.h:164
> +void ewk_view_backing_store_adjuct_visible_rect(Evas_Object*);

May be adjuct replaced with adjust ?

> Source/WebKit/efl/ewk/ewk_view.cpp:2302
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, EINA_FALSE);

Currently we are using false instead of EINA_FALSE.

> Source/WebKit/efl/ewk/ewk_view.cpp:2684
> +void ewk_view_backing_store_adjuct_visible_rect(Evas_Object* ewkView)

May be adjuct replaced with adjust ?

> Source/WebKit/efl/ewk/ewk_view_single.cpp:56
> +    ewk_view_setting_tiled_backing_store_enable_set(ewkView, EINA_TRUE);

Currently we are using true instead of EINA_TRUE.

[Tomasz Morawski]

Created attachment 119998 [details]
Rebased and updated according to KwangHyuk suggestions

Additional change:
in ewk_view_single.cpp:56
Call the ewk_view_setting_tiled_backing_store_enable_set function with true parameter (was false) in _ewk_view_single_smart_add function. Changed this due to that the feature can be only enabled when the --tiled-backing-store compilation flag is used.

[KwangHyuk]

> Source/WebCore/PlatformEfl.cmake:108
> +  IF (WTF_USE_TILED_BACKING_STORE)

Would you check whether WTF_USE_CAIRO would be required for this cairo related files ?

> Source/WebKit/efl/ewk/ewk_private.h:160
> +Eina_Bool ewk_view_setting_tiled_backing_store_enable_set(Evas_Object* ewkView, bool enable);

As it's containing word "tiled", ewk_view_setting_tiled_backing_store_enable_set looks little different from above two APIs
Was it intended ?

> Source/WebKit/efl/ewk/ewk_view.cpp:2296
> +    return EINA_TRUE;

one more EINA_TRUE. :)

[Tomasz Morawski]

(In reply to comment #29)
> > Source/WebCore/PlatformEfl.cmake:108
> > +  IF (WTF_USE_TILED_BACKING_STORE)
> 
> Would you check whether WTF_USE_CAIRO would be required for this cairo related files ?
This "if" is inside IF (WTF_USE_CAIRO) block. Please see: Source/WebCore/PlatformEfl.cmake:84. So, there is no need to add new flag check.

Other things will be fixed.

[Tomasz Morawski]

Created attachment 120159 [details]
Updated

[KwangHyuk]

I don't think that this patch is perfect.
But, it would be helpful for the ewk_single_view. :)

LGTM.

[Tomasz Morawski]

(In reply to comment #18)
> Have the layout tests been run so there are no regressions?
I have made the layout test for disabled and enabled feature. There are not diffrences in result: 8034 tests ran as expected, 500 didn't (19028 didn't run) for both cases. This patch doesn't change layout processing.

[Gyuyoung Kim]

It looks this patch adhere efl port coding style rule well. I think this patch needs to be reviewed by tiled backing store's reviewer as well. It seems Simon Hausmann reviewed tiled backing store.

[KwangHyuk]

I could notice small things.

> ChangeLog:3
> +        [EFL] Use WebCore's tiled backing store in WebKit EFL port

Add '.' into end of title.

> Source/WebCore/ChangeLog:3
> +        [EFL] Use WebCore's tiled backing store in WebKit EFL port

Ditto.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Use WebCore's tiled backing store in WebKit EFL port

Ditto.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:440
> +    int x, y;

Move this close to line 443.

[Gyuyoung Kim]

Comment on attachment 120159 [details]
Updated

Cleared review? from attachment 120159 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Ryuan Choi]

Do you have any plan to update this?

Now, WK2/Efl has WebCore's tiled backingstore.

I hope that WK/Efl also use it instead of ewk_view_tiled.

If you have plan, I will try it.

[Ryuan Choi]

(In reply to comment #37)
> Do you have any plan to update this?
> 
> Now, WK2/Efl has WebCore's tiled backingstore.
> 
> I hope that WK/Efl also use it instead of ewk_view_tiled.
> 
> If you have plan, I will try it.

s/have/don't have/

[Gyuyoung Kim]

(In reply to comment #38)
> (In reply to comment #37)
> > Do you have any plan to update this?
> > 
> > Now, WK2/Efl has WebCore's tiled backingstore.
> > 
> > I hope that WK/Efl also use it instead of ewk_view_tiled.
> > 
> > If you have plan, I will try it.
> 
> s/have/don't have/

I heard that Tomasz had been focused on other project. If there is no response, you can take over this patch.

[Ryuan Choi]

Created attachment 174309 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 174309 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:664
> +    int x, y;

Please move this to 667 line.

> Source/WebKit/efl/ewk/ewk_view.h:2780
> + * Enables/disables the WebCore's tiled backing store

Missing . at the end of line.

> Source/WebKit/efl/ewk/ewk_view.h:2782
> + * @param o view object.

We don't use . in @ fields.

[Ryuan Choi]

Created attachment 174315 [details]
Patch

[Ryuan Choi]

(In reply to comment #41)
> (From update of attachment 174309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174309&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:664
> > +    int x, y;
> 
> Please move this to 667 line.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2780
> > + * Enables/disables the WebCore's tiled backing store
> 
> Missing . at the end of line.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2782
> > + * @param o view object.
> 
> We don't use . in @ fields.

Done

[WebKit Review Bot]

Comment on attachment 174315 [details]
Patch

Attachment 174315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14847217

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Ryuan Choi]

Created attachment 174343 [details]
patch for land

[Ryuan Choi]

(In reply to comment #44)
> (From update of attachment 174315 [details])
> Attachment 174315 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14847217
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

I think that it is not related this bug because I jsut touched efl side.

Anyway, I rebased to see the green bot.

[WebKit Review Bot]

Comment on attachment 174343 [details]
patch for land

Clearing flags on attachment: 174343

Committed r134743: <http://trac.webkit.org/changeset/134743>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.