Bug 75243 - [EFL] Refactor single backing store scroll code.
Summary: [EFL] Refactor single backing store scroll code.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-27 02:13 PST by JungJik Lee
Modified: 2012-01-11 02:06 PST (History)
5 users (show)

See Also:


Attachments
proposal_patch (13.97 KB, patch)
2011-12-27 02:48 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
minor fix (13.97 KB, patch)
2012-01-04 00:58 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
fix minor code (14.02 KB, patch)
2012-01-04 23:42 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
a patch for using memmove api. (13.49 KB, patch)
2012-01-05 04:54 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
new patch via using memcpy and memmove. (13.77 KB, patch)
2012-01-10 02:47 PST, JungJik Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JungJik Lee 2011-12-27 02:13:35 PST
Remove the duplicated code and refactor the scroll procedures.
When we scroll the page, there are one area to repaint and two areas to update.
By following this simple rule, I could refactor and shorten the current code.
Comment 1 JungJik Lee 2011-12-27 02:48:04 PST
Created attachment 120575 [details]
proposal_patch

proposal patch
Comment 2 KwangHyuk 2011-12-27 06:13:28 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=120575&action=review

> Source/WebKit/efl/ewk/ewk_view_single.cpp:94
> +    uint32_t* endOfSource = source + (count - 1);

What about putting (count - 1) into another variable ?

> Source/WebKit/efl/ewk/ewk_view_single.cpp:104
> +    const bool moveLineLeft = sourceX >= destinationX ? true : false;

moveLineLeft = sourceX >= destinationX would be OK.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:111
> +        destination = destinationBegin + (frameWidth * startHeight);

What about putting (frameWidth * startHeight) into another variable ?

> Source/WebKit/efl/ewk/ewk_view_single.cpp:167
> +    int copyHeight = scrollHeight - abs(scrollRequest->dy);

Check whether you can reduce any repetition of same code ?

> Source/WebKit/efl/ewk/ewk_view_single.cpp:172
> +    verticalUpdate.w = scrollWidth - copyWidth;

You can reuse scrollRequest->dx for this.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:179
> +    horizontalUpdate.h = scrollHeight - copyHeight;

You can reuse scrollRequest->dy for this.
What about moving both two Eina_Rectangle block into line 183 ?
And why do you have to use Eina_Rectangle for the temporal usage ?

> Source/WebKit/efl/ewk/ewk_view_single.cpp:181
> +    if (destinationX != sourceX || destinationY != sourceY)

if you have to check the condition, scrollRequest->dx and dy may be used instead of this line.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:190
> +    evas_object_image_data_update_add(smartData->backing_store, scrollX, scrollY, scrollWidth, scrollHeight);

May be, all visible area seems dirty according to implementation.
Would you double check update area ?
Comment 3 JungJik Lee 2012-01-04 00:58:11 PST
Created attachment 121077 [details]
minor fix

fixed the patch by following comments.
Comment 4 KwangHyuk 2012-01-04 07:28:46 PST
> Source/WebKit/efl/ewk/ewk_view_single.cpp:91
> +static inline void _ewk_view_reverse_line_move(uint32_t* destination, uint32_t* source, size_t count)

conut can be const variable.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:105
> +    const bool moveLineLeft = sourceX >= destinationX ? true : false;

sourceX >= destinationX can be true or false. onst bool moveLineLeft = sourceX >= destinationX; would be fine.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:112
> +        destination = destinationBegin + (frameWidth * startHeight);

Why don't you reduce same calculation ? => (frameWidth * startHeight)

> Source/WebKit/efl/ewk/ewk_view_single.cpp:115
> +        if (moveLineLeft)

If you can put the comparison condition outside of for loop, it can reduce cost of repetition of fixed condition check.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:116
> +            memcpy(destination, source, copyWidth * 4);

I suggest you to put copyWidth * 4 outside of for loop.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:171
> +

U better put vas_object_image_data_update_add(smartData->backing_store, destinationX, destinationY, copyWidth, copyHeight) in here.
Comment 5 JungJik Lee 2012-01-04 19:16:23 PST
(In reply to comment #4)
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:91
> > +static inline void _ewk_view_reverse_line_move(uint32_t* destination, uint32_t* source, size_t count)
> 
> conut can be const variable.
> 
I've changed them to const value.

> > Source/WebKit/efl/ewk/ewk_view_single.cpp:105
> > +    const bool moveLineLeft = sourceX >= destinationX ? true : false;
> 
I was considering readability, anyway I've changed it.

> sourceX >= destinationX can be true or false. onst bool moveLineLeft = sourceX >= destinationX; would be fine.
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:112
> > +        destination = destinationBegin + (frameWidth * startHeight);
> 
> Why don't you reduce same calculation ? => (frameWidth * startHeight)
> 
To do so, I have to have another variable to save the value in the loop.

> > Source/WebKit/efl/ewk/ewk_view_single.cpp:115
> > +        if (moveLineLeft)
> 
> If you can put the comparison condition outside of for loop, it can reduce cost of repetition of fixed condition check.
> 
It cannot be outside of the loop so I leave it.

> > Source/WebKit/efl/ewk/ewk_view_single.cpp:116
> > +            memcpy(destination, source, copyWidth * 4);
> 
> I suggest you to put copyWidth * 4 outside of for loop.
> 
I fixed it,too.

> > Source/WebKit/efl/ewk/ewk_view_single.cpp:171
> > +
> 
> U better put vas_object_image_data_update_add(smartData->backing_store, destinationX, destinationY, copyWidth, copyHeight) in here.
I moved it close to _ewk_view_screen_move
Comment 6 JungJik Lee 2012-01-04 23:42:34 PST
Created attachment 121224 [details]
fix minor code

small fixes by comments.
Comment 7 KwangHyuk 2012-01-04 23:47:39 PST
LGTM.
Comment 8 JungJik Lee 2012-01-05 04:54:32 PST
Created attachment 121262 [details]
a patch for using memmove api.
Comment 9 KwangHyuk 2012-01-05 17:11:13 PST
(In reply to comment #8)
> Created an attachment (id=121262) [details]
> a patch for using memmove api.

From the point of code complexity view, it looks really great.

But, IMO, memcpy() doesn't have any special handling for overlapping buffers therefore it may be faster than memmove().

As a result, I suggest you to use memcpy() and memmove() according to the condition.
Comment 10 JungJik Lee 2012-01-06 03:41:27 PST
Unfortunately I understood the original codes' intention and my code would not be so nice. So I am trying to refactor this patch again and testing which one is really fast. Anyway I will file a new patch soon.
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-01-08 13:51:41 PST
Comment on attachment 121262 [details]
a patch for using memmove api.

Clearing flags while the patch is reworked by the reporter.
Comment 12 JungJik Lee 2012-01-08 23:42:25 PST
Comment on attachment 121262 [details]
a patch for using memmove api.

I've tested codes in many cases. Among using memcpy, memmove, memmove in reverse destination and for-loop to copy the values, the for-loop shows the worst timelap in any cases. I've tested it under linux x86 and ARM circumstance. There is not much time gap between memcpy and memmove. Sometimes memcpy shows the best but it wasn't always. If I consider the code readability, I think it would be better using memmove only. So as the result, this patch is not bad as I worried. I add the request to commit-queue again.
Comment 13 JungJik Lee 2012-01-10 02:47:05 PST
Created attachment 121816 [details]
new patch via using memcpy and memmove.
Comment 14 KwangHyuk 2012-01-10 10:10:49 PST
(In reply to comment #13)
> Created an attachment (id=121816) [details]
> new patch via using memcpy and memmove.

LGTM.
Comment 15 Gyuyoung Kim 2012-01-10 23:03:24 PST
Comment on attachment 121816 [details]
new patch via using memcpy and memmove.

From the point of code complexity view, it looks better than before. But, I'm not expert for this area. I'm not sure if there are no problems. Can you make / find proper test cases for this?
Comment 16 JungJik Lee 2012-01-10 23:30:07 PST
(In reply to comment #15)
> (From update of attachment 121816 [details])
> From the point of code complexity view, it looks better than before. But, I'm not expert for this area. I'm not sure if there are no problems. Can you make / find proper test cases for this?

Unfortunately no test case for backing store currently. But it's good point, because I'm thinking about making "run-efl-test.py" in "/Tools/Scripts". Has anyone the same idea in EFL port? if not, I will make the test case soon. BTW, it is safe. :)
Comment 17 Zoltan Herczeg 2012-01-10 23:58:02 PST
Comment on attachment 121816 [details]
new patch via using memcpy and memmove.

r=me
Comment 18 WebKit Review Bot 2012-01-11 02:06:01 PST
Comment on attachment 121816 [details]
new patch via using memcpy and memmove.

Clearing flags on attachment: 121816

Committed r104687: <http://trac.webkit.org/changeset/104687>
Comment 19 WebKit Review Bot 2012-01-11 02:06:07 PST
All reviewed patches have been landed.  Closing bug.