From df304d0155a5f1005798a783b3f8b00cef07f753 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Andre Date: Fri, 13 Oct 2017 17:18:41 +0900 Subject: [PATCH] eo: Add beta API for auto_unref after a call Before screaming in horror (C++...) here's why we may need this: Efl.Part.part API returns an object that is by definition valid for a single function call only. Enforcing this in practice is actually quite hard as all implementation functions must manually take care of the life-cycle. This is a lot of code in many places and a lot of opportunities to forget to properly handle that life-cycle. Also, this means any invalid function call on a part will leak an object. This API absolutely must remain either "internal" or "beta" and definitely not become abused by applications. On top of that such an API can cause great trouble for bindings like C++. As a consequence, only specially crafted APIs like efl_part() should return an object marked as auto_unref. Alternatively this API could be defined in Eo.h or some other Eo_Internal.h. I placed it in efl_object.eo because it's much more convenient :) (read: I'm lazy) **** Performance notes: Tested with clang & gcc (with -O2), I had a look at the output of perf top, in particular the asm view. I used eo_bench in a loop. My conclusions are: - EINA_LIKELY/UNLIKELY actually works. The jump statement varies according to the expectation. I highly doubt all those ugly goto in eo.c / Eo.h are even useful. - The impact of auto_unref on a call_resolve is so small it doesn't even appear in the trace. It is significant inside call_end, though (obviously, that function is just a few lines long). That function accounts for ~1% to ~4% of all CPU time. The impact of auto_unref in call_end is ~4% of the function time. This means ~0.16% of all CPU time (worst measured case). _efl_object_op_api_id_get simply doesn't show up because of caching, so the extra check there is negligible. PS: I also tested EINA_LIKELY/UNLIKELY by compiling with -O2 and looking at the output with objdump. The flag is well respected, and the jump instructions are what you would expect (no jump for LIKELY and jump for UNLIKELY). Conclusion: The goto's in eo.c only make the code harder to read... --- src/lib/eo/efl_object.eo | 23 +++++++++++ src/lib/eo/eo.c | 26 +++++++++++-- src/lib/eo/eo_base_class.c | 11 ++++++ src/lib/eo/eo_private.h | 1 + src/tests/eo/suite/eo_test_general.c | 57 ++++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/lib/eo/efl_object.eo b/src/lib/eo/efl_object.eo index b1595081b4..ceef80e08c 100644 --- a/src/lib/eo/efl_object.eo +++ b/src/lib/eo/efl_object.eo @@ -279,6 +279,29 @@ abstract Efl.Object () even if @.parent is not $null.]] } } + @property auto_unref @protected @beta { + [[Mark an object to be automatically deleted after a function call. + + This becomes effectives only after finalize is done. After any EO + function has been called on this object, it will be unref'ed. This + property will also be reset to $false. This is intended to simplify + Part objects lifecycle. + + Note: This applies to any EO function call, even if the object was + of the wrong type (call resolution failed). + + This is a write-only property as reading it would unref the object, + and reset the flag. + + Warning: This is a beta API, do not use it unless you know exactly + what you are doing. + ]] + set {} + values { + enable: bool(false); + [[If $true, unref this object after the next call.]] + } + } } implements { class.constructor; diff --git a/src/lib/eo/eo.c b/src/lib/eo/eo.c index f567ab755b..17da1115f9 100644 --- a/src/lib/eo/eo.c +++ b/src/lib/eo/eo.c @@ -16,6 +16,9 @@ # include #endif +#define EFL_OBJECT_BETA +#define EFL_OBJECT_PROTECTED + #include "Eo.h" #include "eo_ptr_indirection.h" #include "eo_private.h" @@ -616,6 +619,11 @@ err_func_src: err: if (is_obj) { + if (EINA_UNLIKELY(obj->auto_unref != 0)) + { + if (!(--obj->auto_unref)) + efl_unref(eo_id); + } _efl_unref(obj); _eo_obj_pointer_done((Eo_Id)eo_id); } @@ -670,6 +678,11 @@ _efl_object_call_end(Efl_Object_Op_Call_Data *call) { if (EINA_LIKELY(!!call->obj)) { + if (EINA_UNLIKELY(call->obj->auto_unref != 0)) + { + if (!(--call->obj->auto_unref)) + efl_unref(call->eo_id); + } _efl_unref(call->obj); _eo_obj_pointer_done((Eo_Id)call->eo_id); } @@ -721,19 +734,26 @@ _efl_object_api_op_id_get(const void *api_func) } EAPI Efl_Object_Op -_efl_object_op_api_id_get(const void *api_func, const Eo *obj, const char *api_func_name, const char *file, int line) +_efl_object_op_api_id_get(const void *api_func, const Eo *eo_obj, const char *api_func_name, const char *file, int line) { Efl_Object_Op op; #ifndef EO_DEBUG - if (!obj) return EFL_NOOP; + if (!eo_obj) return EFL_NOOP; #endif op = _efl_object_api_op_id_get_internal(api_func); if (op == EFL_NOOP) { + EO_OBJ_POINTER(eo_obj, obj); eina_log_print(_eo_log_dom, EINA_LOG_LEVEL_ERR, file, api_func_name, line, - "Unable to resolve op for api func %p for obj=%p (%s)", api_func, obj, efl_class_name_get(obj)); + "Unable to resolve op for api func %p for obj=%p (%s)", api_func, eo_obj, efl_class_name_get(eo_obj)); + if (EINA_UNLIKELY(obj->auto_unref)) + { + if (!(--obj->auto_unref)) + efl_unref(eo_obj); + } + return EFL_NOOP; } return op; diff --git a/src/lib/eo/eo_base_class.c b/src/lib/eo/eo_base_class.c index 7b6f915225..8eda874cb2 100644 --- a/src/lib/eo/eo_base_class.c +++ b/src/lib/eo/eo_base_class.c @@ -5,6 +5,9 @@ #include #include +#define EFL_OBJECT_BETA +#define EFL_OBJECT_PROTECTED + #include "Eo.h" #include "eo_ptr_indirection.h" #include "eo_private.h" @@ -2106,6 +2109,14 @@ _efl_object_allow_parent_unref_get(Eo *obj_id EINA_UNUSED, Efl_Object_Data *pd) return pd->allow_parent_unref; } +EOLIAN static void +_efl_object_auto_unref_set(Eo *obj_id EINA_UNUSED, Efl_Object_Data *pd EINA_UNUSED, Eina_Bool enable) +{ + // Write-only property + EO_OBJ_POINTER(obj_id, obj); + obj->auto_unref = enable ? 2 : 0; +} + EOLIAN static Eo * _efl_object_finalize(Eo *obj, Efl_Object_Data *pd EINA_UNUSED) { diff --git a/src/lib/eo/eo_private.h b/src/lib/eo/eo_private.h index 93fcba97dd..676b0fbab6 100644 --- a/src/lib/eo/eo_private.h +++ b/src/lib/eo/eo_private.h @@ -119,6 +119,7 @@ struct _Eo_Object Eina_Bool del_triggered:1; Eina_Bool destructed:1; Eina_Bool manual_free:1; + unsigned char auto_unref : 2; // unref after 1 call }; /* How we search and store the implementations in classes. */ diff --git a/src/tests/eo/suite/eo_test_general.c b/src/tests/eo/suite/eo_test_general.c index 9a26ffea42..3c21e71c14 100644 --- a/src/tests/eo/suite/eo_test_general.c +++ b/src/tests/eo/suite/eo_test_general.c @@ -9,6 +9,9 @@ # include #endif +#define EFL_OBJECT_BETA +#define EFL_OBJECT_PROTECTED + #include #include "eo_suite.h" @@ -1719,6 +1722,59 @@ START_TEST(efl_cast_test) } END_TEST +static void +_auto_unref_del_cb(void *data, const Efl_Event *ev EINA_UNUSED) +{ + *((int *) data) = 1; +} + +START_TEST(efl_object_auto_unref_test) +{ + int _auto_unref_del; + Eo *obj, *parent; + + efl_object_init(); + + // Test unref after valid call + _auto_unref_del = 0; + obj = efl_add(SIMPLE_CLASS, NULL); + fail_if(efl_ref_get(obj) != 1); + efl_event_callback_add(obj, EFL_EVENT_DEL, _auto_unref_del_cb, &_auto_unref_del); + efl_auto_unref_set(obj, 1); + fail_if(_auto_unref_del); + fail_if(efl_ref_get(obj) != 1); + efl_name_set(obj, "name"); + fail_if(!_auto_unref_del); + + // Test unref after invalid call + _auto_unref_del = 0; + obj = efl_add(SIMPLE_CLASS, NULL); + fail_if(efl_ref_get(obj) != 1); + efl_event_callback_add(obj, EFL_EVENT_DEL, _auto_unref_del_cb, &_auto_unref_del); + efl_auto_unref_set(obj, 1); + fail_if(_auto_unref_del); + fail_if(efl_ref_get(obj) != 1); + simple_no_implementation(obj); + fail_if(!_auto_unref_del); + + // Same with a parent + _auto_unref_del = 0; + parent = efl_add(SIMPLE_CLASS, NULL); + obj = efl_add(SIMPLE_CLASS, parent); + fail_if(efl_ref_get(obj) != 1); + efl_allow_parent_unref_set(obj, 1); + efl_event_callback_add(obj, EFL_EVENT_DEL, _auto_unref_del_cb, &_auto_unref_del); + efl_auto_unref_set(obj, 1); + fail_if(_auto_unref_del); + fail_if(efl_ref_get(obj) != 1); + efl_name_set(obj, "name"); + fail_if(!_auto_unref_del); + efl_del(parent); + + efl_object_shutdown(); +} +END_TEST + void eo_test_general(TCase *tc) { tcase_add_test(tc, eo_simple); @@ -1744,4 +1800,5 @@ void eo_test_general(TCase *tc) tcase_add_test(tc, eo_rec_interface); tcase_add_test(tc, eo_domain); tcase_add_test(tc, efl_cast_test); + tcase_add_test(tc, efl_object_auto_unref_test); }