Page MenuHomePhabricator

optimize glyph images data copy into 4 byte aligned images
ClosedPublic

Authored by ali.alzyod on Dec 16 2018, 3:35 AM.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8445
Build 7591: arc lint + arc unit
ali.alzyod created this revision.Dec 16 2018, 3:35 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Dec 16 2018, 3:35 AM

small optimization for how image data is copied between two images:
1- if possible copy all image data at once
2- else copy data row by row,

@ali.alzyod
Thank you for your first patch!
I leaved a inline comment. Please check that comment. :)
And please check the following things.

  1. Code convention. https://www.enlightenment.org/contrib/devs/coding-conventions
  1. Commit message. https://www.enlightenment.org/contrib/devs/git-guide
src/modules/evas/engines/gl_common/evas_gl_font.c
29

The ndata which is assigned memory using alloca() will be free'ed at end of this function.
That means, the address of ndata is not going to be stored in evas_gl_common_texture_alpha_new() function.
So, I think, if nw is same with w, you can use the address of data directly. without calling alloca().
But, I'm not sure about this. Please, test it, too. :)

if (nw == w)

{
    ndata = data;
}

else

{
    ndata = alloca(nw * h);  
    if (!ndata) return NULL;
    // and existing for loop code...
}

ignore allocating memory if we do not have to.
just assign it to new pointer (used only for read only)

ali.alzyod marked an inline comment as done.

I'm curious... in what kind of benchmark/situation did this actually make a measurable difference? :) I never saw this function in any profiles (well not near the top of anything) so it never was looked into.

This will have small increase effect in performance (still may be it is good to have )

In rare cases it can be measured, example : if we got huge glyphes (font size is very big), and text block have lots of glyphes (characters) then we can cut a lot of memory copy time in CPU.

yeah - but what are these benchmarks? where does this ACTUALLY happen enough?

ali.alzyod retitled this revision from optimize image data copy 1-copy all data at once if possible 2-copy image data row by row to optimize glyph images data copy into 4 byte aligned images.Dec 17 2018, 4:58 AM
ali.alzyod added a comment.EditedDec 20 2018, 5:19 AM

On my Machine i7-2600 Ubuntu 16.04 LTS

Time spent on copy memory for rendering following text:
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890"

With following style:
DEFAULT='font=arial font_size=512 color=#000 wrap=char text_class=entry'

Time Spent:
Old Version of code 18 ms
New Version of code 0.07 ms

The change is simple 2 parts:
1- if we do not need to copy, then avoid copy (image is already 4 byte align)
2- Instead of copy byte by byte in for loop, use memory copy (memcpy) function to copy chunks of memory together (image rows)

ok. that's something more concrete. 512 is a huge size... like insanely huge. but ok - some info as to the kind of sizes you are looking at. thanks!

I set font size just to show the effect without writing a lot of text (instead of many glyphes we have huge one ),
and effect of copy will increase when image size increase.

I run test with Font size 72
With following text:

evas_object_textblock_text_markup_set(tb,
         "<b>ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz</b>"
         "<i>ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz</i>"
         "<b font=serif>ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz</b>"
         "<i font=serif>ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz</i>"
         );

Old Version of code 1.3 ms
New Version of code 0.2 ms

by the way,
May be huge font size could be used on some application for TVs (for example learning apps for kids)|
Just a thought :), but sure it is not common in normal apps (unless you like to zoom alot)

ManMower accepted this revision.Jan 9 2019, 12:11 PM
ManMower added a subscriber: ManMower.

Don't really see a reason not to like this. :)

This revision is now accepted and ready to land.Jan 9 2019, 12:11 PM
This revision was automatically updated to reflect the committed changes.