Page MenuHomePhabricator

edje_load: Delete unnecessary style update function call.
AbandonedPublic

Authored by CHAN on Apr 5 2019, 1:26 AM.

Details

Summary

The textblock styles are loaded at the same time when edje is loaded.
And if I change the color class or text class, it will update again.

I think that if all the widgets are created and the theme is changed, updating the whole style at this point is seen as a waste of resources.

When i check the memory heap state, launch time of the certain application. It takes a lot of heap.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10762
Build 8365: arc lint + arc unit
CHAN created this revision.Apr 5 2019, 1:26 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/

CHAN requested review of this revision.Apr 5 2019, 1:26 AM
CHAN edited the summary of this revision. (Show Details)Apr 5 2019, 1:31 AM
CHAN added reviewers: Hermet, woohyun, bowonryu, herdsman.
CHAN edited the summary of this revision. (Show Details)Apr 5 2019, 1:40 AM
CHAN edited the summary of this revision. (Show Details)
cedric requested changes to this revision.Apr 5 2019, 9:51 AM

Hum, this doesn't seems like a good idea to me. This means that the style won't be set on all the textblock. It would be interesting to know why there are so many of them. Looking at evas_object_textblock.c in _efl_canvas_text_style_set, I don't think the style cache is working at all as it does just a '==' between two strings that have no guarantee to be stringshare. Maybe adding a strcmp there would help.

This revision now requires changes to proceed.Apr 5 2019, 9:51 AM
CHAN edited the summary of this revision. (Show Details)Apr 7 2019, 9:13 PM
CHAN requested review of this revision.Apr 8 2019, 12:45 AM

@cedric Thank you for reviewing.

Oops, The Tizen Theme has a code diff to support text and color classes

The situation with the upstream is slightly different. Forget the memory heap size i previously attached.

Anyways, textblock sytle is set when edje is loaded, and all update is also done for text class changes.
Is there any reason for all update here when theme_set is called?

In D8560#155710, @CHAN wrote:

@cedric Thank you for reviewing.

Oops, The Tizen Theme has a code diff to support text and color classes

The situation with the upstream is slightly different. Forget the memory heap size i previously attached.

Anyways, textblock sytle is set when edje is loaded, and all update is also done for text class changes.

Is there any reason for all update here  when theme_set is called?

I would guess, from memory, that this is due to custom style. If none are set by the user (no call to gfx text_class_set), but we still have some defined inside the edje file, it should be applied. I don't think we can really remove them. Just optimizing the cache would be good enough in my opinion.

CHAN abandoned this revision.Thu, Apr 25, 2:42 AM

@cedric Thanks for giving opinion.

It would be good to discuss the improvement with the relevant person in charge.

Thanks.