Page MenuHomePhabricator

evas/textblock: optimize layouting when textblock has empty text
Needs RevisionPublic

Authored by smohanty on Aug 8 2019, 7:55 PM.

Details

Summary

Layouting is expensive as it does the format parsing. I see even we do
fomatting when evas_textblock has no text to layout. by putting this simple
check we can delay the layouting till we have some text to layout.

Note: this layouting happens a lot if edje part is textblock.

 Am not sure sould we return EINA_FALSE or EINA_TRUE in the case
 when we don't do layouting. But bottom line is by putting this simple
check we will stop doing lot of unnecessary format parsing/ layouting.

Diff Detail

Repository
rEFL core/efl
Branch
textblck_empty_opt
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12624
Build 9082: arc lint + arc unit
smohanty created this revision.Aug 8 2019, 7:55 PM

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/

smohanty requested review of this revision.Aug 8 2019, 7:55 PM
smohanty updated this revision to Diff 23990.Aug 8 2019, 9:35 PM

build fix.

Hermet requested changes to this revision.Aug 8 2019, 10:11 PM

What if text is changed to none from actual text, relayout shouldn't be performed?

This revision now requires changes to proceed.Aug 8 2019, 10:11 PM
ali.alzyod added a comment.EditedAug 8 2019, 10:25 PM

Hello @smohanty how are you ?

As @Hermet mention textblock will not be able to remove old content if text changed to empty string.

#define EFL_EO_API_SUPPORT 1
#define EFL_BETA_API_SUPPORT 1
#include <Eina.h>
#include <Efl.h>
#include <Elementary.h>

EAPI_MAIN int
elm_main(int argc, char **argv)
{
   Evas_Object *win, *textblock;

   elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);

   win = elm_win_util_standard_add("Main", "App");
   elm_win_autodel_set(win, EINA_TRUE);

   evas_object_textblock_text_markup_set(textblock, "<font=Sans color=#000 font_size=30>Hello</font>");
   //Force Layout
   evas_textblock_cursor_line_char_first(evas_object_textblock_cursor_get(textblock));
   evas_object_textblock_text_markup_set(textblock, "");
   evas_object_size_hint_weight_set(textblock, EVAS_HINT_EXPAND, EVAS_HINT_EXPAND);
   evas_object_size_hint_align_set(textblock, EVAS_HINT_FILL, EVAS_HINT_FILL);
   evas_object_show(textblock);

   evas_object_move(textblock, 0, 0);
   evas_object_resize(textblock, 640, 800);
   evas_object_resize(win, 640, 800);

   evas_object_show(win);
   elm_run();

   return 0;
}
ELM_MAIN()

Also inside TEXT block we need to create at least one empty paragraph even if there are no text.

And layouting will happen only once if content/style stay the same, So I think it is not big deal

@ali.alzyod ,

Am doing great thanks. how about you ?

Yes am not sure about the internal working of the Textblock. But Textblock does lot of format parsing even if the text is empty ( or to put it in other way it does reformatting even there is no change in format)

So my main intention is to reduce number of formatting . @ali.alzyod , Please feel free to add a separate commit to address that issue (As formatting does memory allocation we need do do it only when its absolutely needed )

@ali.alzyod ,

The use case am talking about happens a lot when textblock part is created from edje.  to reproduce the issue create a empty window application and add a button and put some logs in the _format_fill() to see how many times we do reformatting.

@ali.alzyod ,

The use case am talking about happens a lot when textblock part is created from edje.  to reproduce the issue create a empty window application and add a button and put some logs in the _format_fill() to see how many times we do reformatting.

I think if issue happens because of edje, then optimization should happens in edje, There are many stuff goes there (It will create/remove TEXTBLOCK objects) not only working on one TEXTBLOCK object

@ali.alzyod

I think its a issue in Textblock . If the text style hasn't changed then it shouldn't do reformatting the style text. If you need a proof i can create a test case and send it to you which does reformatting although style hasn't changed.
I printed the style text most of the time it was same. and i didn't found any code that checks if style test has changed .. if anything triggers relayouting it just blindly parse the text without checking if the style has changed.

@ali.alzyod

I think its a issue in Textblock . If the text style hasn't changed then it shouldn't do reformatting the style text. If you need a proof i can create a test case and send it to you which does reformatting although style hasn't changed.
I printed the style text most of the time it was same. and i didn't found any code that checks if style test has changed .. if anything triggers relayouting it just blindly parse the text without checking if the style has changed.

Yes please,Can you please provide any test code, to show the issue, (If it can C only without edje, then it will be great) ?

just put this code block in your main() function and see how many times we do style text reformatting.

Note : I set the style once at start.

 int l , r, t, b;
 Evas_Object * txt = evas_object_textblock_add(evas_object_evas_get(win));

 Evas_Textblock_Style *st = evas_textblock_style_new();
 evas_textblock_style_set(st, "DEFAULT='font=Sans font_size=16 color=#114 wrap=word'");
 evas_object_textblock_style_set(txt, st);

 evas_object_textblock_style_insets_get(txt, &l, &r, &t, &b);
 evas_object_textblock_text_markup_set(txt, "Hello world! :)");
 evas_object_textblock_style_insets_get(txt, &l, &r, &t, &b);
 evas_object_textblock_text_markup_set(txt, "Hello worlda! :)");
 evas_object_textblock_style_insets_get(txt, &l, &r, &t, &b);
 evas_object_textblock_text_markup_set(txt, "Hello worldab! :)");
 evas_object_textblock_style_insets_get(txt, &l, &r, &t, &b);

@smohanty thanks for the example, For me now it is more clear where is the problem.

By the way :
I do not think this patch will effect such cases, where no empty case happened. That's way I did not get it first time :)
even with:

evas_object_textblock_style_insets_get(textblock, &l, &r, &t, &b);
evas_object_textblock_text_markup_set(textblock, "");
evas_object_textblock_style_insets_get(textblock, &l, &r, &t, &b);
evas_object_textblock_text_markup_set(textblock, "");
evas_object_textblock_style_insets_get(textblock, &l, &r, &t, &b);
evas_object_textblock_text_markup_set(textblock, "");
evas_object_textblock_style_insets_get(textblock, &l, &r, &t, &b);

There are no extra lay-outing happened

@ali.alzyod ,

 Thanks for the optimisation patch :) . Can only test on Monday .

  Actually these unnecessary relayouting() triggers happened from edje_textblock. please have look at _edje_part_recalc_single_textblock_min_max_calc() function maybe you can find out few more issues there.
  
 I didn't investigate much in to it. but what i doubt if we call  efl_gfx_entity_size_set(); and then   efl_canvas_text_size_formatted_get() or efl_canvas_text_size_native_get() which triggers a relay-outing even if there is no text to layout in the textblock. 

Will try to reproduce these issue in a simple app and if reproduce it will report to you.
 I will close this one . and will create a new one regarding layouting with empty text as a new issue.

@smohanty ,
I will take a look at edje parts, but the main issue with edje it has its own logic where it keep set markup_set evas_object_textblock_text_markup_set and for some reason it will force textblock to relayout and create text items. I suspect that the reason set inside Edje logic not the TextBlock, but anyway lets discuss them next week :)

cedric requested changes to this revision.Aug 9 2019, 9:29 AM

I see actually that this discussion should lead to new test being added to evas textblock test and maybe also a benchmark. Please in general, provide an easy scenario for testing your improvement. Patch to expedite are also acceptable.

kimcinoo edited the summary of this revision. (Show Details)Aug 11 2019, 7:25 PM
Hermet resigned from this revision.Aug 13 2019, 12:07 AM