Page MenuHomePhabricator

eo: implement class overriding (+unit tests)
ClosedPublic

Authored by zmike on Jan 18 2019, 1:53 PM.

Details

Summary

this enables an app or a platform to add an override for a given class,
then return a different object when that class is created. the benefit is
that a class can be internally customized by the app without needing to
modify upstream versions of that class

@feature
fix T7516

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zmike created this revision.Jan 18 2019, 1:53 PM
zmike requested review of this revision.Jan 18 2019, 1:53 PM
bu5hm4n requested changes to this revision.Jan 30 2019, 3:19 AM
bu5hm4n added a subscriber: bu5hm4n.

I am not too sure about that tbh. a lookup in such a hash gets quite expensive once the hash has a few elements.

Further more i am not too sure if this is a good idea and should not be solved differently, just by inheriting the class, and adding a new constructor, we could add a shortcut for that to eo.

Further more, you "class_get" symbols are weak symbols, which means they can be overridden from a app. Even though i don't think that this is a good idea, i guess we could try to use it that way instead of a hash which could potentially hamper our performance ...

This revision now requires changes to proceed.Jan 30 2019, 3:19 AM

I am not too sure about that tbh. a lookup in such a hash gets quite expensive once the hash has a few elements.

I am not convinced that this will be a performance problem as the construction of an object is a very slow task in all situation. The overhead of looking up in a hash won't have much impact on this. The main cost of looking up in a hash is the computation of the key, this can be avoided when there is no entry in the hash which is an easy optimization to add.

Further more i am not too sure if this is a good idea and should not be solved differently, just by inheriting the class, and adding a new constructor, we could add a shortcut for that to eo.

This is not just a solution for inheriting a class, but for being able to completely replace it in the inheritance chain.

Further more, you "class_get" symbols are weak symbols, which means they can be overridden from a app. Even though i don't think that this is a good idea, i guess we could try to use it that way instead of a hash which could potentially hamper our performance ...

Application overriding a class is not the only use case. The logic is to be able for system provided eo module to also override a class in a system wide solution. This should enable behavior that are device specific and don't make much sense upstream.

I would not say that this is a slow path. I remember a very small change to efl_add, the marking of EINA_LIKELY / EINAUNLIKELY or EINA_COLD / EINA_HOT changing a lot in that regard. And even if it is a simple hash looking here, it is going to be calculated on EVERY single object that is created, which is a lot. So adding one single override for one single class is going to impact every other object creation. But before we discuss here more, can we have a benchmark for that ? :)

I can see what you want to achive with this. But i still think that the better alternative is the following:

  • Create a inheriting class from the class (A) you want to change
  • override the A_class_get function with the class you have just created.
  • EFL will now not create A objects anymore but rather your inherited modulized class
  • No performance / call tree impact for the rest of classes that you might call / use.

I would not say that this is a slow path. I remember a very small change to efl_add, the marking of EINA_LIKELY / EINAUNLIKELY or EINA_COLD / EINA_HOT changing a lot in that regard. And even if it is a simple hash looking here, it is going to be calculated on EVERY single object that is created, which is a lot. So adding one single override for one single class is going to impact every other object creation. But before we discuss here more, can we have a benchmark for that ? :)

That was a micro benchmark. Pretty sure that this has zero impact on the speed of efl_add at the end as we allocate the object, resolve the call to constructor and call the entire chain, and then resolve the call to finalize and call the entire chain, plus generate a few events on the way. Compared to that, a hash is really not on the heavy side of things.

Indeed, would be neat to have some benchmark that test with Efl.Object instead of the current benchmark which create a simple class and benchmark with it.

I can see what you want to achive with this. But i still think that the better alternative is the following:

  • Create a inheriting class from the class (A) you want to change

I think that part can not work as it will call the overridden A_class_get to generate the inheritance tree. It is not a big deal, I think, but better known this limitation.

  • override the A_class_get function with the class you have just created.
  • EFL will now not create A objects anymore but rather your inherited modulized class

That is indeed a benefit of this idea.

  • No performance / call tree impact for the rest of classes that you might call / use.

Agreed, but I am not sure how to get a system wide module providing system wide widget override working. It has to be in a different .so than any libe*.so. I guess that maybe we could get that working with a LD_PRELOAD or having the quicklaunch server preload module manually. Maybe.

@zmike after ediscussing this in RL we came up with the idea of adding this to _efl_add_internal_start, so we don't call there directly the class_get function but rather check if there is an override class_get function in a hash table and call that instead.

zmike updated this revision to Diff 19172.Feb 4 2019, 9:41 AM
zmike retitled this revision from eo: implement class constructor overriding (+unit tests) to eo: implement class overriding (+unit tests).

rework to do class overriding

bu5hm4n requested changes to this revision.Feb 4 2019, 10:03 AM

Just this tiny NIT. Looks good otherwise! :)

src/lib/eo/eo.c
2616

Do we want to warn / error when there is already a override ?

This revision now requires changes to proceed.Feb 4 2019, 10:03 AM
zmike requested review of this revision.Feb 4 2019, 10:13 AM
zmike added inline comments.
src/lib/eo/eo.c
2616

No, because then if e.g., a system provides a global override and an app provides an app override, the app cannot remove the global override in any way.

bu5hm4n accepted this revision.Feb 4 2019, 10:30 AM
This revision is now accepted and ready to land.Feb 4 2019, 10:30 AM
Closed by commit rEFLc946b1477dca: eo: implement class overriding (+unit tests) (authored by zmike, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyFeb 4 2019, 10:31 AM
This revision was automatically updated to reflect the committed changes.