Bug 75243 – [EFL] Refactor single backing store scroll code.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[JungJik Lee]

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.

[JungJik Lee]

Created attachment 120575 [details]
proposal_patch

proposal patch

[KwangHyuk]

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 ?

[JungJik Lee]

Created attachment 121077 [details]
minor fix

fixed the patch by following comments.

[KwangHyuk]

> 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.

[JungJik Lee]

(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

[JungJik Lee]

Created attachment 121224 [details]
fix minor code

small fixes by comments.

[KwangHyuk]

LGTM.

[JungJik Lee]

Created attachment 121262 [details]
a patch for using memmove api.

[KwangHyuk]

(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.

[JungJik Lee]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 121262 [details]
a patch for using memmove api.

Clearing flags while the patch is reworked by the reporter.

[JungJik Lee]

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.

[JungJik Lee]

Created attachment 121816 [details]
new patch via using memcpy and memmove.

[KwangHyuk]

(In reply to comment #13)
> Created an attachment (id=121816) [details]
> new patch via using memcpy and memmove.

LGTM.

[Gyuyoung Kim]

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?

[JungJik Lee]

(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. :)

[Zoltan Herczeg]

Comment on attachment 121816 [details]
new patch via using memcpy and memmove.

r=me

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.