Page MenuHomePhabricator

eo/vtable: Fix issue with vtable creation of class.
Needs RevisionPublic

Authored by smohanty on Aug 26 2019, 3:02 AM.

Details

Summary

For any given class currently we copy the vtable data from the root to the
parent of the klass. As already the parent class has done the same for itself
we can just copy from the parent class no need to build from scratch.

The issue with building from scratch is that we will endup with more unique table
because of table_merge logic and unique slot index.

testenv : elementary_test
This patch creates 55 fewer table than before = 26KB in 64bit sysytem.

Diff Detail

Repository
rEFL core/efl
Branch
table
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13043
Build 9276: arc lint + arc unit
smohanty created this revision.Aug 26 2019, 3:02 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/

smohanty requested review of this revision.Aug 26 2019, 3:02 AM
tasn requested changes to this revision.Aug 26 2019, 6:21 AM

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.

src/lib/eo/eo.c
874

Please wrap it with explicit parenthesis. Extra important for nested expressions (tbh should always do it).

This revision now requires changes to proceed.Aug 26 2019, 6:21 AM
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.

Let me explain why _vtable_copy_all() will do a copy although parent has the identical table.

Example :

struct A {
    virtual void func() { printf("A::func "); }
};

struct B : public A{
    virtual void func() { printf("B::func "); }
};

struct C : public B{
};

With the Current Implementation.
when we copy table for C class.

  1. first we copy table of A (refcount 2)
  2. copy table of B (as table is already exists we do merge and both table content are not same so we make a new table and merge the data which is nothing but the B table content)

Do you see the problem ?

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.

The idea is not to skip the parent , instead start the copy operation from parent . and sometimes parent is NULL so your suggestion will not work.

smohanty updated this revision to Diff 24509.Aug 26 2019, 5:32 PM

updated review comment.

smohanty marked an inline comment as done.Aug 26 2019, 5:32 PM
tasn added a comment.Aug 27 2019, 1:07 AM
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.

The idea is not to skip the parent , instead start the copy operation from parent . and sometimes parent is NULL so your suggestion will not work.

My suggestion was wrong anyway (wrong equal sign), but now it's corrected and also account to my misunderstanding about copy from parent:

for (  ; *mro_itr && (*mro_itr != klass->parent) ; mro_itr++) ;
if (klass->parent == NULL) {
   mro_itr--;
}

I hope it's now clear what I had in mind.

src/lib/eo/eo.c
874

Sorry, I meant wrap the if, not just the break.

tasn added a comment.Aug 27 2019, 1:16 AM

Let me explain why _vtable_copy_all() will do a copy although parent has the identical table.

Example :

struct A {
    virtual void func() { printf("A::func "); }
};

struct B : public A{
    virtual void func() { printf("B::func "); }
};

struct C : public B{
};

With the Current Implementation.
when we copy table for C class.

  1. first we copy table of A (refcount 2)
  2. copy table of B (as table is already exists we do merge and both table content are not same so we make a new table and merge the data which is nothing but the B table content)

    Do you see the problem ?

I honestly don't remember what's there in Eo and I don't know anyway because things have changed. I guess this code was created for mixins because they could modify previously defined vtables areas and their changes would need to be merged. I guess it's the same for interfaces though. I just don't remember the details. Have you tried adding code that compares your created vtable with the previously created vtable (just create both and compare)? Because I wouldn't be surprised if it would fail.

Just one example: let's assume a world where we have just one class called A (with one function) and one interface called I which are completely unrelated. We now want to create a new class called B that inherits from the class and implements the interface (doesn't add any new functions). The vtable will therefore look something like:

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

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?

smohanty updated this revision to Diff 24516.Aug 27 2019, 1:25 AM

updated review 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?

Yes I understand that point it will fail if the MRO list keeps the interface first and then the class hierarchy. in that case that will be another issue to fix .

The proper fix is to copy the vtable of the parent and then copy the vtable of all extensions . (As current implementation has one MRO list with class hierarchy and extension i just checked the implementation to see the order and then put the patch as extensions always comes after the
class hierarchy ).

smohanty marked an inline comment as done.Aug 27 2019, 1:37 AM
tasn added a comment.Aug 27 2019, 1:45 AM

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?

Yes I understand that point it will fail if the MRO list keeps the interface first and then the class hierarchy. in that case that will be another issue to fix .

The proper fix is to copy the vtable of the parent and then copy the vtable of all extensions . (As current implementation has one MRO list with class hierarchy and extension i just checked the implementation to see the order and then put the patch as extensions always comes after the
class hierarchy ).

It will always keep the interface first, so it will always fail. As it should btw. I don't see how you can avoid this, these interfaces actually do change the MRO, they actually do change those vtables. Have you tried my suggesting of comparing the vtables to see if you broke them with this change? Because I think every change you suggested (and can suggest) will break this.

src/lib/eo/eo.c
871

Please don't lose the "skip ourselves" comment.

smohanty updated this revision to Diff 24518.Aug 27 2019, 2:36 AM

added back the comment.

smohanty marked an inline comment as done.Aug 27 2019, 2:37 AM
tasn requested changes to this revision.Aug 27 2019, 4:45 AM

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.

This revision now requires changes to proceed.Aug 27 2019, 4:45 AM
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.

I have tested all the MRO list generated in _eo_class_mro_init() which keeps class hierarchy first and then the extension list. A simple static code analysis of _eo_class_mro_init() will be enough if thats what actually the case or not. If we don't keep the MRO list in that order then it will be simple fix inside _eo_class_mro_init() function.

If you show me a usecase where the order is indeed what you are suggesting then i will raise a separate patch to fix that issue.

Andif you are not sure about all possible usecase maybe you can recommend someone else who knows about the Eo to review the scenario you are talking about.

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)

The only issue is when a class has both base class and mixin . then the MRO list order should be mixin ->parent class hirachy-> cur class.

So only check we have to do in the mro_init() function if thats indeed the case or not.

tasn added a comment.EditedAug 27 2019, 5:52 AM

I have tested all the MRO list generated in _eo_class_mro_init() which keeps class hierarchy first and then the extension list. A simple static code analysis of _eo_class_mro_init() will be enough if thats what actually the case or not. If we don't keep the MRO list in that order then it will be simple fix inside _eo_class_mro_init() function.

If you show me a usecase where the order is indeed what you are suggesting then i will raise a separate patch to fix that issue.

There is no issue with the mro list, the issue is with your suggested change, at least if my memory is correct (and I'm pretty sure it is).

If a class A implements a mixin M and an interface I and inherits from a class B, the MRO is:

A, M, I, B. (clarification edit: I meant mental MRO, not the actual one saved by eo)

With your code M won't be applied correctly (and maybe also I) , so I don't see what you are trying to say here. Since isa is implemented as a function, this should also affect interfaces, not just mixins, though it's quite possible the "ISA" is implemented elsewhere so the issue is just with mixins.

Andif you are not sure about all possible usecase maybe you can recommend someone else who knows about the Eo to review the scenario you are talking about.

I'm quite sick and tired with your combative attitude. I'm trying to help you and you keep on being an asshole. This is not the first time.

Just to turn this statement on you: if you don't know what's going on there, please stop sending patches that break things. If you do know what's going on there, then you'd either see that what I'm saying above is correct and then your patch is wrong, or will know that what I'm saying is wrong, and then it would be easy to correct me.

It's quite simple, savings don't come from nowhere. If this change of yours actually saves something, it means, by definition, that it's doing something different. Now, what we are trying to figure out here is what it's doing differently. So the question is, what? Looking at your code it's not handling mixins. It looks like the mixins vtables will not be copied (or refcounted) into the main class which is bad. It can either cause a performance regression (if it adds the need to walk the mro to resolve the function) or actual breakage.

What's even worse is that you are suggesting such an intrusive change without strong understanding of what's going on there, a carefree attitude, no tests to make sure you're not breaking anything, and no benchmark results to make sure you haven't hit performance. This code is at the core of the EFL, every small change can affect everything in weird and surprising ways.

Rest of the topic will discuss later.

If a class A implements a mixin M and an interface I and inherits from a class B, the MRO is:

A, M, I, B. (clarification edit: I meant mental MRO, not the actual one saved by eo)

With your code M won't be applied correctly (and maybe also I) , so I don't see what you are trying to say here. Since isa is implemented as a function, this should also affect interfaces, not just mixins, though it's quite possible the "ISA" is implemented elsewhere so the issue is just with mixins.

@tasn ,

Could you clarify the above statement ?
Did you mean that the above class hierarchy will break because of my patch (then I can write one test case to simulate that class hierarchy)
or

Its just your mental model not actual MRO list saved by Eo.

confused by this line ( > A, M, I, B. (clarification edit: I meant mental MRO, not the actual one saved by eo))
tasn added a comment.Aug 29 2019, 7:52 AM

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.

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.

Sorry for the badly formatted question.

All I was asking if my patch will break the above scenario/ test case you mentioned . As I understood now it will not break.

smohanty updated this revision to Diff 24632.Aug 29 2019, 6:05 PM

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

Test Done :

 efl eo test suite passed.
elementary_test example works.

Taken a Snapshot of Vtable array (after flattening of function array. )
Before Patch and after patch the slots that are filled are same. attached the log

Before :


After:

tasn requested changes to this revision.Aug 30 2019, 6:58 AM

What am I seeing in these logs? How were they collected? This is not the only problem, you should also be comparing the functions themselves and see that they are not different, not just what you're comparing. The whole problem is that you're not doing the mixin.

Also, your current code adds allocations, array searches and a lot of inefficiency for a code that's supposedly identical to the previous version you had here...

This revision now requires changes to proceed.Aug 30 2019, 6:58 AM
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

In D9739#182086, @tasn wrote:

you should also be comparing the functions themselves and see that they are not different, not just what you're comparing.

comparing against what ? I know you must have wrote some unit test cases to catch that issue . so i just run those unit test cases.
If you missed to add those cases let me know I will add it myself.

In D9739#182086, @tasn wrote:

The whole problem is that you're not doing the mixin.

Show me the test case , if you haven't written one write one and show that it fails , then will fix my patch.

In D9739#182086, @tasn wrote:

Also, your current code adds allocations, array searches and a lot of inefficiency for a code that's supposedly identical to the previous version you had here...

Well the whole issue with the previous patch is that it didn't fit your mental MRO ( your whole complain was that I must have skipped some of the table to copy )

I am glad that you figure it out (both patch are identical in terms of logic) . I just put it so that your imaginary mental MRO will not break.

Note: As we both agree there is a Bug here , It dose not matter who fix it. I don't mind if you want to raise a proper patch to fix it . But this issue has to be fixed .

smohanty requested review of this revision.Aug 30 2019, 5:19 PM
tasn requested changes to this revision.Aug 31 2019, 2:59 AM

We don't agree there is a bug here. Maybe there is one, but I'm not yet convinced. I'm also not going to review this for at least a week. Your shit attitude doesn't deserve my immediate attention. I was trying to help you with a quick turnaround, but now, you don't deserve it anymore.

This revision now requires changes to proceed.Aug 31 2019, 2:59 AM

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.

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.

@raster ,
Thanks for bringing it up .. If you have some time please read the whole conversation to see who is getting personal . His whole attitude from the start is that this patch is totally wrong and am yet to receive a constructive review comment from him all I get is I shouldn't touch Eo code because I don't have experience in Eo. As you see from his last comment He has not even acknowledged that there is a Bug here.

There is nothing am going to gain personally if this patch will go in upstream . I found a bug and fixed it and thought it is a value addition for upstream.

Having said that
I am not here to get anybody's shit just because I found a bug and put a patch to fix it.

If you want I will stop raising patches for upstream . But I can't entertain these kind of review comments (with no substance or any constructive review comment) .