On Mon, May 9, 2011 at 7:32 PM, Gustavo Sverzut Barbieri
<[email protected]> wrote:
> On Mon, May 9, 2011 at 2:03 PM, Cedric BAIL <[email protected]> wrote:
>> On Tue, May 3, 2011 at 8:19 PM, Gustavo Sverzut Barbieri
>> <[email protected]> wrote:
>>> On Tue, May 3, 2011 at 6:17 AM, Cedric BAIL <[email protected]> wrote:
>>>> On Sat, Apr 30, 2011 at 11:59 PM, Gustavo Sverzut Barbieri
>>>> <[email protected]> wrote:
>>>>> On Sat, Apr 30, 2011 at 1:09 PM, Tom Hacohen <[email protected]> wrote:
>>>>>> On Sat, Apr 30, 2011 at 5:59 PM, Gustavo Sverzut Barbieri
>>>>>> <[email protected]> wrote:
>>>>>>> On Fri, Apr 29, 2011 at 3:54 PM, Enlightenment SVN
>>>>>>> <[email protected]> wrote:
>>>>>>> > Log:
>>>>>>> > Eina refcount: Wrap EINA_REFCOUNT_UNREF with do {} while(0).
>>>>>>> > #define EINA_REFCOUNT_UNREF(Variable, Free_Callback) \
>>>>>>> > - if (--((Variable)->__refcount) == 0) \
>>>>>>> > - Free_Callback(Variable);
>>>>>>> > + do \
>>>>>>> > + { \
>>>>>>> > + if (--((Variable)->__refcount) == 0) \
>>>>>>> > + Free_Callback(Variable); \
>>>>>>> > + } \
>>>>>>> > + while (0)
>>>>>>>
>>>>>>> maybe think of a way to set Free_Callback automatically? Spread this
>>>>>>> all around the code will be bad?
>>>>>>
>>>>>> I don't really agree. If the free callback is type specific then you'll
>>>>>> get
>>>>>> a compilation warning
>>>>>> when doing something wrong and how different is it from just calling our
>>>>>> custom
>>>>>> unref_my_special_item?
>>>>>>
>>>>>> I.e it's
>>>>>> EINA_REFCOUNT_UNREF(item, free_my_item);
>>>>>> v.s
>>>>>> unref_my_item(item);
>>>>>>
>>>>>> Both are error-prone the same way.
>>>>>>
>>>>>>
>>>>>> Coming to think about it, the current way is not good because it breaks
>>>>>> inheritance.
>>>>>> Here's an example of something that will break:
>>>>>>
>>>>>> struct parent { EINA_REFCOUNT; int foo; };
>>>>>>
>>>>>> struct child { struct parent p; int bar; };
>>>>>>
>>>>>> there's no sane way to do
>>>>>> struct child item;
>>>>>> ...
>>>>>> EINA_REFCOUNT_UNREF(item, free_my_item);
>>>>>> because child->__refcount doesn't exist and passing &child->parent
>>>>>> instead
>>>>>> will cause
>>>>>> a compilation warning when calling free_my_item...
>>>>>>
>>>>>> I guess we should drop this refcount thingie, too much hassle and issues.
>>>>>
>>>>> I'm all for dropping it.
>>>>
>>>> I disagree, it's an improvement over having nothing and doing it
>>>> everywhere by our own. With just this macro you have at least a common
>>>> pattern that you can easily understand and track, not something
>>>> depending on the dev who did the job. And more we will use
>>>> Ecore_Thread, more people will need refcount and more we will have
>>>> different implementation of the exact same thing.
>>>
>>> Here I disagree based on the following technical reasons:
>>> - you're macro is not thread safe, thus not usable freely with
>>> threads. You rely on specific behavior that does not touch the
>>> reference count at the same time in two threads;
>>
>> Well, for most case, you refcount in the main loop. See current user
>> of it, both refcount stuff in the main loop and unref them also in the
>> main loop. That's the common case. I can add a thread safe refcount
>> later, that would be another type. But in my opinion it's useless with
>> Ecore_Thread worker, you do increase the refcount when you start the
>> worker, not when it is running or you end up with reference somewhere
>> in the pipe that are not counted. So that make it more complex.
>
> This is not written anywhere. I know that because I followed the
> development, but I'm pretty sure random eina user will do a nasty
> mistake and curse you forever :-)
Yeah, I know, need to put that into docs and tutorial... Typically
this should be part of simple Ecore_Thread sample tutorial. It's on my
TODO... Should raise it's priority, sure.
>>> - if you make this thread-safe, then you'd waste resources on
>>> common case that does not require it;
>>
>> Agreed.
>>
>>> - the code you're "abstracting" is way too simple. Maybe it's worth
>>> to have some coccinele check for the reference count + free if you
>>> want to detect mistakes.
>>
>> Yes, it is simple. And in my opinion, making it look the same
>> everywhere is better than having to run a special tool to check that
>> it indeed the same every where (because nobody will run that tool
>> often, because every one will have to re-understand how code that are
>> almost the same work).
>
> The problem is that abstracting it under a macro/function will lead
> people to consider it "magic" and always work. Particularly with
> reference counting, as most (all?) other solutions work in thread
> context.
Well, how do they do this kind of stuff on Apple system, because the
way they abstract thread with Grand Central Dispatch is almost the
same we do with Ecore_Thread. As far as I know, glib and Qt
implementation is more like a raw abstraction of pthread than a worker
make it easy model (I didn't look at what they did since years).
> Note, in some architectures you have atomic inc/dec that would remove
> one problem without locks, but you still would have to test the value
> later on the unref case.
Yes, ref/unref with thread safe require some mutex or a special hardware help.
>>>>> Really, I already told that in the past when Andre Dieb was doing
>>>>> EUPnP he asked for similar thing and I said "no, don't go that way,
>>>>> it's not worth"
>>>>
>>>> The cost is 0, the maintenance is 0 also and it improve readability
>>>> and maintenance of the code using it in my opinion. Making it an
>>>> improvement.
>>>
>>> I don't agree with that. See above.
>>
>> Well, as for maintenance, you didn't say anything about that. From my
>> point of view, maintenance is 0, because you don't have to do anything
>> in eina anymore. And maintenance in the app is improving as you have
>> the same logic for the code and you can reuse what you already know
>> from a previous experience insteed of understanding how this behaviour
>> was coded that time.
>
> But consider the above. Also in some cases you don't even need
> references, rather can go with some binary "in_use" flag. Evas, for
> instance, worked with that for a decade :-)
Evas wasn't using thread for a long time. The first one that used
thread was preloading and later come async rendering... and now we are
in a middle of a mess. That piece of code, Evas_Cache_Image need a
complete rewrite in my opinion as it became a hell of a maintenance.
REF/UNREF wouldn't have change everything, but it would have help.
> Leaving these things to the user is simpler for now. Simpler than
> trying to cover all cases in one. And really, I never noticed people
> doing manual reference counting incorrectly in that aspect. Sure,
> people forget to ref/deref, but they never do stupid things that these
> simple macros try to avoid.
I know people that don't want to use refcounting, because they fear
the complexity of them and use lock, boolean and all other kind of
work around, that make the code unusable. I also know people that did
some dummy stuff with refcounting like leaking some data because of a
wrong test. Of course this is just example and doesn't make the
solution relevant. But at least, I think that this ref/unref macro
does fit the Ecore_Thread model pretty well and make it possible to
simplify the code and make it more readable for all of us.
>>>>> also, with eina_object, that I dislike the name as in Eina it's better
>>>>> to just have a "memory handle" instead objects. Objects will lead
>>>>> people to expect complex things like 1) inheritance; 2) interfaces; 3)
>>>>> reference counting; 4) type safety (or checking). Putting all of those
>>>>> in C and in making it easy-to-use is impossible... being tried over
>>>>> the past 30 years without success :-P
>>>>
>>>> Right now, Eina_Object is limiting itself to something like
>>>> inheritance (more just like C structure inheritance by packing parent
>>>> on top of the structure), type safety (that was the main purpose of
>>>> it) and memory repacking. I am now planning to add refcount.
>>>
>>> you also need some concept of class x instance. Lots of definitions
>>> are easier done per-class and will avoid memory waste and useless
>>> setup cycles. Similar to Evas_Smart x Evas_Object.
>>
>> Yes, Eina_Object have a complex of Eina_Class of course.
>
> Hum, ok. Also thinking about this ref/unref reminded me of other
> issues with it: how do you handle circular dependencies? Either you
> have a GC running in a thread (bohem-gc) or you'll need 2 phase
> termination like GObject (finalize x destroy). All of this smells like
> more problem than solution :-/
Hum. I don't remember how I implemented some stuff, but my idea was to
force only parent/child relation. Avoiding graph and forcing a tree
like relation. This avoid the problem with GC as you can create a loop
in a tree. I am just wondering if I implemented this correctly. Need
to check that.
>>>> I don't like the idea of adding interfaces to it as it is where the
>>>> cost come most of the time, but that's open for discussion.
>>>
>>> Interface is just another pointer in the type (class) definition, It
>>> will contain a list of other types (kludge) or interface definitions
>>> (structure with pointers + identifier).
>>
>> Ok, I maybe miss understand that term, but for me the interface is
>> where come all the hugly function with virtual and other inheritance
>> stuff. So yes, only a list of pointer with an identifier, but you need
>> to provide a way to handle all the thing that people expect, and that
>> have a cost that I don't see us making it better than what the other
>> have done.
>
> virtuals are already handled in our Class thing (it it looks like
> Evas_Smart_Class). Interfaces are a list that are similar to this
> Class, with extension points. It's a way to solve the
> multiple-inheritance, to solve "this is a widget (class) that is
> scrollable (interface)". Then when you call
> elm_scrollable_policy_set(object), the elm_scrollable_policy_set would
> be implemented like:
>
> void ABC_scrollable_policy_set(ABC_Object *obj, int horiz, int vert) {
> const ABC_Interface *iface;
> const ABC_Scrollable_Iface *siface= NULL;
> ABC_Class *cls;
> const Eina_List *l;
> CHECK_OBJECT_VALIDITY(obj);
>
> // this should be a function on its own:
> cls = GET_OBJECT_CLASS(obj);
> EINA_LIST_FOREACH(cls->interfaces, l, iface)
> if (iface->type = ABC_SCROLLABLE_INTERFACE)
> {
> siface = iface;
> break;
> }
> if (!siface) return; // does not implement interface
>
> // dispatch it to the interface... you could omit siface as you
> can retrieve it again from obj
> siface->policy_set(siface, obj, horiz, vert);
> }
Current idea of implementation would make it more like :
void ABC_scrollable_policy_set(ABC_Object *obj, int horiz, int vert) {
ABC_Scrollable_Iface *siface = NULL;
ABC_Scrollable *o;
o = GET_OBJECT(obj, ABC_Scrollable_Iface);
siface = GET_CLASS(obj, ABC_Scrollable_Iface);
if (!siface) return ; // does not implement interface
siface->sfunc.policy_set(siface, o, horiz, vert);
}
The main difference is that ABC_Scrollable strcture would look like :
struct {
ABC_Object obj;
ABC_Scrollable_Specific scroll;
};
And the ABC_Scrollable_Iface would look like :
struct {
ABC_Object_Func ofunc;
ABC_Scrollable_Func sfunc;
};
Dispatching could be done when creating the class, but I don't know
how to implement this at all.
--
Cedric BAIL
------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
