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 )

In D11488#219055, @bu5hm4n wrote:

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

In D11488#219006, @ali.alzyod wrote:

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

indeed solves the issue.

The change to be opaque should really happen once. Only if you switch back and forth for each frame will you continuously allocate memory. Basically you fallback to the case where you do not have any cow.

As I am running an animation (using lottie to render to the buffer ) so each frame I update the buffer (part of evas_image) with below Apis.

@cedric ,
Regarding my use case , I am updating image data in every frame by calling evas_object_image_data_get() and evas_object_image_data_set() api . I don't know how Image object can figure out if the content is opaque or not if user has set some pixel data (as it will be expensive to go through each pixel to figure it out) . So maybe we can modify the is_opaque() to return false when user has set some pixel data.

@cedric ,
What do you think ?

Thanks for the detailed explanation after reading it got little bit more understanding of the current issue with edje memory consumption issue.

If you guys knows the history of this code, please let me know your opinion.

In D9739#182258, @raster wrote:

hey @smohanty - keep things from getting personal. we DO have guidelines on how to behave: https://www.enlightenment.org/contact . You're not joking here and it's definitely crossing a line.

In D9739#182086, @tasn wrote:

What am I seeing in these logs? How were they collected?

Taken a Snapshot of Vtable array (after flattening of function array. ) by just printing the content of chain array (0 - not filled , 1 means filled)
Before Patch and after patch the slots that are filled are same. attached the log

Note for future review comment.

In D9794#182033, @tasn wrote:

I don't get this change. Readability is subjective and this doesn't read any more readable to me, and it's 20 lines longer (not that it's the only parameter, but still).
It's also hard to follow when you changed a few unrelated things in one commit (or is it just phab being bad?).

Test Done :

 efl eo test suite passed.
elementary_test example works.

updated the patch so that it will be a better match to the mental model.

In D9739#181840, @tasn wrote:

Could you please fix the formatting in your comment? I'm not sure what you're trying to say there because of the wrong quote and etc. Regarding the MRO: I know it's not the actual MRO, delete the I from there then, it's still the same.

Rest of the topic will discuss later.

@ali.alzyod ,
I don't have access to submit the patch so you have to submit it.

updated review comment

I think you are optimising wrong place . Please put some log and run elementary_test and first see how many heap allocation is happening and whats the average size of those strings , then you will get a better understanding about the nature of the problem and how to fix it.

Regarding the Interface order , Issue is not with the Interface as they don't have any implementation , so they will correctly get filled during the function_set() time (if the slot is empty the table will get created)

In D9739#181101, @tasn wrote:

Thanks for updating the diff, though as I described above, the whole change is probably problematic and I don't see how you can fix it without completely changing the approach.

added back the comment.

The mro for B is B, I and then A. So with your code it'll just skip I's vtable (which is all the slots empty other than slot[2]), and will make the vtable look like:

slot[0] = _A_isa
slot[1] = _A_func
slot[2] = NULL
slot[3] = _B_isa

See the problem I was talking about?

updated review comment.

@bu5hm4n ,

Did you get any chance to look at this issue ?

In D9719#180913, @ali.alzyod wrote:

but my test app shows font size 10 for both l1 and l2 .. I think that itself is a problem.

on trial and error, I notice override text_class (even on object level) will effect all objects that uses same style.
So l1 and l2 will be 10 (last override value)

This example shows what I am talking about
https://github.com/Ali-Alzyoud/override-text_class

updated review comment.

In D9739#180896, @tasn wrote:

Regardless of that, even if the behaviour wasn't wrong, this change could probably be done by replacing line 843 with for ( ; *mro_itr && (*mro_itr == klass->parent) ; mro_itr++) ; instead all of the added code. Then line 846 will still do the correct thing because it'll skip either ourselves or parent.

In D9739#180896, @tasn wrote:

I unfortunately don't remember the exact details of the implementation, but if memory serves, this doesn't actually do a copy, but rather just increase the refcount. Actually, I just checked and it's indeed the case. So if for some reason it's not doing refcounting correctly, that should be fixed. Though the behaviour proposed by this change is wrong.

Regardless of that, even if the behaviour wasn't wrong, this change could probably be done by replacing line 843 with for ( ; *mro_itr && (*mro_itr == klass->parent) ; mro_itr++) ; instead all of the added code. Then line 846 will still do the correct thing because it'll skip either ourselves or parent.

Though as I said, this change is wrong. Just read through _vtable_copy_all and you can see that it's wrong.

updated docs wording from Xavi Artigas.

updated review comment

Let concentrate on below snippet.

In D9719#180651, @ali.alzyod wrote:

@smohanty
I am referring to EDC file you are using, to test text_class, can you share it ?

it seems you can not override text_class for text_block unless you added the text_class inside the style used in EDC file.

updated review comment.

In D9719#180612, @ali.alzyod wrote:

@smohanty ,

Can you try to add text_class into style, instead of add description in text section ?

style 
      { 
         name: "tb_style";
         base: "font=sans font_size=30 color=#FF0 text_class=custom_text_class";
      }

In D9719#180355, @zmike wrote:

@smohanty Are you doing anything after those calls? You might accidentally be triggering an edje recalc that reapplies the default class.

In D9715#180358, @zmike wrote:

I think the list -> array should be broken out into a separate patch (using git-phab). Also some other minor nits. Seems fine otherwise though.

@ali.alzyod
I am trying to add a test case to test the below code in edje , but don't know which api to call to test it.