On Thu, Jan 11, 2018 at 5:56 AM, Jean-Philippe André <j...@videolan.org> wrote: ... >> if we wish to do cascade deletion, which may be very helpful (delete a >> loop, get rid of all users... like done for evas + objects), then we >> can dissociate terminate/invalidate from reference, but the last >> reference would invalidate: >> >> --> NOTE: using efl_new() instead of efl_add() as I think this should >> be renamed for clarity... >> >> basic usage: >> o = efl_new(cls, parent, ...); // refcnt = 1, the caller's reference >> efl_unref(o); // refcnt = 0, efl_invalidate(o) is auto-called >> >> >> explicit invalidate: >> o = efl_new(cls, parent, ...); // refcnt = 1, the caller's reference >> efl_invalidate(o); // refcnt = 1 >> efl_unref(o); // refcnt = 0, no call to efl_invalidate(o) >> >> canvas usage #1 (basic usage): >> o = efl_new(EFL_UI_BUTTON_CLASS, window, ...); // refcnt = 1, the >> caller's reference >> efl_unref(o); // refcnt = 0, efl_invalidate(o) is auto-called, >> button is removed from window >> >> canvas usage #2 (invalidate window): >> o = efl_new(EFL_UI_BUTTON_CLASS, window, ...); // refcnt = 1, the >> caller's reference >> efl_invalidate(window); // o.refcnt = 1, but efl_invalidate(o) is >> called >> efl_unref(o); // refcnt = 0, no call efl_invalidate(o) >> >> problem with this approach is that most people will forget to >> efl_unref(o), then while it will be invalidated, its memory will leak. >> >> while this can be solved within evas legacy wrapper, the usability is >> not the best. Other systems "solve" this with one of two options: >> >> - reference transference: efl_adopt_ref(receiver, o), would give your >> reference of "o" to "receiver", which is supposed to release it on its >> "invalidate". That is: keep an Eina_List *adopted, that is >> automatically managed and efl_unref() on its own invalidate. >> >> - floating references: object starts "unowned" and the first "user" >> should own it, for details: >> https://developer.gnome.org/gobject/stable/gobject-The-Base- >> Object-Type.html#floating-ref >> >> GObject UI and gstreamer use this "floating" and I always disliked it, >> finding it too error prone. It "looks easy" to use, but the situation >> is similar to what we have in EFL and led to this thread... so I'd >> avoid it. >> >> since unlike Gtk and GStreamer we're suggesting elements to be created >> with a parent, as opposed to adding to a parent, this is simpler. >> Compare: >> >> // GTK: >> container = create_container (); >> container_add_child (container, create_child()); >> >> // EFL: >> container = efl_new(CONTAINER_CLS, loop, ....); >> child = efl_new(CHILD_CLS, container, efl_container_pack(container, >> efl_added), efl_adopt_ref(container, efl_added)); >> >> this is explicit, clear what's happening. Since "efl_adopt_ref()" was >> written explicitly there, or called afterwards, it's easy to spot you >> don't own the reference anymore. Maybe coccinelle, coverity and the >> likes can told that rule to spot errors. >> > > ah i really don't like efl_adopt_ref > > > others may claim efl_adopt_ref() is always the case, in that case >> keeping efl_invalidate() UNRELATED to efl_unref() is also helpful: >> >> --> NOTE: using efl_add(), since here parents keep the reference >> >> WRONG/BUG usage: >> o = efl_add(cls, parent, ...); // refcnt = 1, the PARENT's reference >> efl_unref(o); // refcnt = 0, but parent held the reference >> > > so this should ERR like now?
YES, but unref/ref will be demoted, not handled like it's done now. basically now we all handle efl_unref() at some point, as Andrew said it's confusing when you need to do or not. to "spot for bugs" it will be possible to grep efl_ref/efl_unref ;-) to mitigate those, my recommendation is to generate new eoid in efl_ref() and invalidate it on the efl_unref()... like if they were 2 different handles, but they point to the same underlying object. prevents too-much-unref, since the handle will be invalid. we can have the compiler to help by using proper function attributes, like done for malloc/free, warning unused results, etc. > basic usage: >> o = efl_add(cls, parent, ...); // refcnt = 1, the PARENT's reference >> efl_invalidate(o); // refcnt = 0, since parent will unref based on >> invalidate event >> >> cascade delete usage: >> o = efl_add(cls, parent, ...); // refcnt = 1, the PARENT's reference >> efl_invalidate(parent); // refcnt = 0, cascade invalidate, leading >> to parents unref children. >> >> ref usage (when you can't monitor EFL_EVENT_INVALIDATE or weak-ref): >> o = efl_add(cls, parent, ...); // refcnt = 1, the PARENT's reference >> efl_ref(o); // refcnt = 2, PARENT + caller >> efl_invalidate(o); // refcnt = 1, since parent will unref based on >> invalidate event >> efl_unref(o); // refcnt = 0 >> > > probably more like: > efl_ref(o) > efl_invalidate(o) -> triggers "invalidate" > some "invalidate" cb calls efl_unref(o) yes, this is what happens underneath, but since you "efl_ref(o)", you should "efl_unref(o)" as well. YOU. and if my suggestion about eoid above is implemented, my example would ERROR! It should be more correctly written as: o = efl_ref(o); efl_invalidate(o); efl_unref(o); since you're only supposed to unref the reference you own (the result from efl_ref(o)), not what existed before. > one question is: > - why keep a ref? how useful is a ref'ed object that has been invalidated? > if you can't call any function on it, then what's the point of the ref? the > eo id indirection already makes most invalid calls safe from crashes and > show ERR > if you can call functions, then maybe invalidate must do very little work > (eg. evas: hide+mark for deletion, io/net: close fd) or it'll render > objects unsafe to use > parent objects may listen to "invalidate" events from their children and > unparent them for cases where you can't weakref or listen for events and invalidate handles. that's the single usage I see, THUS why I suggest the eoid alias and "demote" its usage. with this clear approach, all my usages like below are pointless: efl_ref(o); efl_event_callback_call(o, ...); // may call delete on me, nasty efl_other_check(o); // may be invalid/deleted efl_unref(o); this WON'T be needed anymore. Same for loops that releasing stuff may release yourself. it won't happen since I own the reference to "o". from efl_callback_call(), what people may call is efl_invalidate(), which will invalidate the object but KEEP reference alive, so efl_other_check() won't crash or raise warnings (it should check if the object is invalidated to do something useful). as I said, "grep for efl_ref()" will be our guide to spot bugs ;-) it shouldn't be needed, unless in rare cases. > should we consider: > - efl_new() - no parent - you own the ref > - efl_add() - always a parent - you don't own any ref - the only way to > create evas objects (they need a parent, always, except the window) windows should need a loop, at least. The rare case where a parent is not needed is likely the Efl.Task/Efl.Process/Efl.Thread (but I'd make Efl.Thread point back to Efl.Process...). Rasterman wants to collapse those in one, so the "root" element would be the Efl.Loop. In that case, loop would be the only entity to not have a parent. recall without a parent you can't create timers, idlers, jobs, fd monitors... basically you can't do much, just synchronous/blocking CPU tasks. > - efl_add_ref() - always a parent + you get a ref (for bindings) not not sure if this is neeed. NOTE: Bindings shouldn't need this. Bindings usually keep a this.wrapped_object, or self.wrapped_object... which can use weak-ref or EVENT_DEL to make this pointer null, preventing further calls to that. at least the original Python bindings never needed those, of course they weren't available at that time either ;-) > - efl_invalidate() - no ref change on itself - evas parents will unparent > and recursively also invalidate their children. ok, but I don't get the statement below: > unparenting is left to the > individual classes (efl.canvas.object being the one for all of evas) why is that? why do you need unparenting? > - efl_unref() - if 0 without a parent, calls efl_invalidate() if needed & > destroy I think this can happen, but it's highly NOT recommended. if parents rely on "efl_unref()" to invalidate... others may keep the reference, and then object is NOT invalidated when the author expects. May lead to bugs. if they always call "efl_invalidate()", then it's clear the object must be invalidated at that moment. what I'd do in the base class implementation (which could be overridden for custom behavior) is let the parent automatically remove its reference to that child (the one he automatically got with efl_add()). Maybe the process to acquire the ref and later release this ref should be implemented in virtuals, so can be more easily traced/debugged and overridden. > - if 0 with a parent, ERR but invalidate & destroy (like now) "if 0 with a parent" == bug, ERR and do nothing. since the parent owns its ref, somebody unreffed too much. We may help this to be clear with new eoid per efl_ref(). > - efl_del() - is it needed? is it invalidate + unref? ( remove, or make this == efl_invalidate() and make clear no refs are removed on YOUR behalf (which is confusing due the "del" name). It may remove references held by OTHERS (ie: parent, others that efl_ref()...). Then if YOU did efl_ref(), you efl_unref() AFTER efl_invalidate(). > then: > - parent_set() always adds a ref (so we can get rid of the parent_sunk > madness) > - parent_set(null) removes the parent ref > - parent_set(other) transfers a ref, refcount unchanged should we be able to unparent/reparent? Seems dangerous in most cases. Do we have some clear usecases? Shouldn't we offer a way to create a new object based on that one? in Efl.Net.Server I allow that to happen, you can "unparent/reparent" a connection, so you "steal" that from the server, allows the server to be closed/destroyed with the connection remaining alive. But I doubt there are use cases for that, and it "works easily" for file descriptors, but more complex resources may be a can of worms. in Evas, one could reparent widgets from a container to another... but not from a window to another. maybe these shouldn't be in the core objects, rather have some kind of transfer/copy interfaces that some objects may opt to implement. > the base class also presents: > - invalidator > - destructor > to be implemented by most subclasses > > > anyway it is clear that not everyone agrees on the idea that all objects > must have a parent. > that's indeed an unusual requirement for an object model (think any OO > language) indeed the whole Eo and lifecycle is OOP optimized for Efl usage, so it's not standard, but address real life issues. > for efl, the loop object can trivially be found using a TLS so that > shouldn't be a blocker. this should be prohibited if we ever want the whole multi-loop to be done right... thus parent and loop provider thing must be enforced. Not global/tls... > one more point to consider is evas object manual_free, but i don't think > any of the above can fix this: > - invalidator (called by efl_invalidate) just hides and triggers an event > - but no ref change > - destructor (called when refs = 0) remains unchanged: delete most things > but keep internal evas object data safe for 2 frames for cur/prev stuff > - manual_free done by evas itself. already 0 ref here I never get the idea behind manual_free, but looks more like a hack. Maybe if you need to keep resources alive, evas could keep an extra ref. On invalidate, it would schedule the "2 frame safety" and once that is reached, the extra ref is removed. > as a last note to this long email, shall we opt to use the "parent >> keeps a ref" method, since we use the eoid abstraction, I'd highly >> recommend that efl_ref() creates ANOTHER eoid for the same object, >> with associated WARN_UNUSED_RESULT/MALLOC and efl_unref() behaves like >> free(), considering the handle to be destroyed. This should help >> finding bugs... it's worth since efl_ref() will be rarely used. >> > > interesting idea... hard to do in practice without changing our existing > 344 use cases of efl_ref :) at least compilers will bitch. But as I said, fixing the behavior may get rid with some of those, at least all that I wrote in efl.net for sure. -- Gustavo Sverzut Barbieri -------------------------------------- Mobile: +55 (16) 99354-9890 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
