Page MenuHomePhabricator

edje: Fix regression in edje textblock style handling.
AcceptedPublic

Authored by smohanty on Mar 11 2020, 6:55 PM.

Details

Summary

The regression is caused due to recent changes in text_class override in object level
which changed the whole idea of lazy generation of styles only when needed.
this patch undo those changes.

Now for global, file and object level text_class change we will just invalidate the
edje style cache so that when next request to the style_get() function called we
will recompute the style with the new changes.

Diff Detail

Repository
rEFL core/efl
Branch
textblock
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16276
Build 10911: arc lint + arc unit
smohanty created this revision.Mar 11 2020, 6: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.Mar 11 2020, 6:55 PM
woohyun accepted this revision.Mar 11 2020, 9:17 PM
woohyun added a reviewer: ali.alzyod.

This looks good for me, but I hope @a.srour and @ali.alzyod will also check together.

This revision is now accepted and ready to land.Mar 11 2020, 9:18 PM

@smohanty @woohyun
Is the purpose of this patch speed ? or is there another issue?

@smohanty @woohyun
Is the purpose of this patch speed ? or is there another issue?

The main issue is functionality broken in the tizen product size .. I didn't investigate all the reasons but one thing is clear as we keep a copy of the style per edje . when text_class changes in file level or global level we just make the file level styles cache dirty (not the edje level cache ). thats why some cases changing the text_classes and color_classes in global level doesn't update the view .

and second case is yes Optimization that we don't have to traverse all the styles in a File (for example default theme has more than 300 styles ) .

ali.alzyod added a comment.EditedMar 12 2020, 4:16 AM

@smohanty @woohyun
Is the purpose of this patch speed ? or is there another issue?

The main issue is functionality broken in the tizen product size .. I didn't investigate all the reasons but one thing is clear as we keep a copy of the style per edje . when text_class changes in file level or global level we just make the file level styles cache dirty (not the edje level cache ). thats why some cases changing the text_classes and color_classes in global level doesn't update the view .

hmmm, I am not sure, but I think this is may happen due to design purpose, for example if user overrides text_class at an object level it will update, and then override the text_class at file level, this will not re-update the object again.

since this pass ninja test, I am ok with it
But if you can share any test case showing the issue (to produce it), it will be great

Would you mind adding tests that ensure the behavior that is needed ? otherwise this will break again at some point i guess :/

Would you mind adding tests that ensure the behavior that is needed ? otherwise this will break again at some point i guess :/

I don't know where exactly the test case should be added . But testing the performance impact is very straight forward.

Below is the code snippet where have created 10 buttons and on click of last button updated global buton text_style 10000 times.

static void
_global_text_class_change(void *data, Evas_Object *obj, void *event_info EINA_UNUSED)
{
   int i = 0;
   for(i = 0; i < 10000; i++){
      int j = i % 40 + 20;
      edje_text_class_set("button", "Rosemary", j);
   }
}

main()
{
 ...
   Evas_Object * btn;
   int count = 0;
   for (; count < 10; count++) 
   {
      btn = elm_button_add(win);
      elm_object_text_set(btn, "button 1");
      elm_box_pack_end(bx, btn);
      evas_object_show(btn);      
   }
   evas_object_smart_callback_add(btn, "clicked", _global_text_class_change, btn);
}

Result :
It takes 600ms the callback to run in my ubuntu machine .. so can be easily 10 times slower in target.
Most of the time spend in list_data_get() and strcmp() and rest of them i think the memory access.
Vtune Screenshot :

and with this patch same use case it takes around 80ms mostly due to the Hash Search of Efl_Provider which is same for both cases. ( anyway hash iterator is another story )

bu5hm4n added a comment.EditedMar 13 2020, 1:04 AM

Okay, your earlier comments sounded to me like this broke some behavior which is restored in this revision.
Even if its straight forward to test for the performance, can you add that snipped to ./src/benchmarks, the next one touching these parts might want to test for that benchmark as well.

EDIT: In case there *is* something with behavior, ./src/tests/ is the place to add this.