Oh, another point. I'm pretty sure that the many very ugly goto we have in the code to "not pollute L1 cache" are in fact not useful. I had a look at the ASM produced by GCC -O2 and it strictly follows EINA_(UN)LIKELY. If goto is used for error handling, fine, but if it's used with "goto label_back", readability is really bad, while apparently not having any meaningful impact on the binary code (for GCC, clang differs a bit). EINA_LIKELY is good enough, IMHO. And hardware branch prediction happens no matter what.
So, for at least Eo.h I think it would be very good to remove the goto. On Tue, Jan 16, 2018 at 7:53 PM, Jean-Philippe André <[email protected]> wrote: > Hi, > > This broke make check (I fixed it now). > > Note that this patch probably should not have been merged as a single > commit, as the Phab Diff contains multiple patches. > > Best regards, > > On Tue, Jan 16, 2018 at 5:50 PM, Jean Guyomarc'h <[email protected]> > wrote: > >> raster pushed a commit to branch master. >> >> http://git.enlightenment.org/core/efl.git/commit/?id=34d9f20 >> 70696027199a56cb621c0526ea1430e8f >> >> commit 34d9f2070696027199a56cb621c0526ea1430e8f >> Author: Jean Guyomarc'h <[email protected]> >> Date: Tue Jan 16 14:58:38 2018 +0900 >> >> eina: remove usless newline >> >> Summary: >> ecore_evas: remove debug >> >> eina: unregister log level when done with >> >> Fixes a constant memory leak. >> >> eina: introduce EINA_HOT and EINA_COLD >> >> These attributes respectivelly expand to __attribute__ ((hot)) and >> __attribute__ ((cold)) when available. They allow to mark functions >> are >> being hot/cold (frequently used or not) as well as to qualify labels >> within a function (likely/unlikely branches). >> >> eo: speed-up generated calls by removing call cache >> >> The call cache needed to by thread-local, to avoid concurrency issues. >> Problem with TLS is that is adds an extra overhead, which appears to >> be >> greater than the optimization the cache provides. >> >> Op is naturally atomic, because it is an unsigned integer. As such, it >> cannot be tempered with while another thread is reading it. When >> entering the generated function, the first operation done is reading >> 'op'. If we have concurrency, we will have access sequences returning >> either EFL_NOOP or a VALID op, because 'op' is not set until the very >> end of the function, when everything has been computed. As such, we >> now >> use the 'op' atomic integer to instore a lock-free/wait-free >> mechanism, >> which allows to drop the TLS nature of the cache, speeding up the >> access >> to the cache, and therefore making functions execute faster. >> >> We don't test anymore the generation count. This can be put as a >> limitation. If means that if you call efl_object_shutdown() and >> re-initialize it later with different data, opcodes will be invalid. >> I am not sure there is any usecase for this to ever happen. >> We could move all the caches in a dedicated section, that can be >> overwritten after a call to efl_object_shutdown(), but I am not sure >> it >> will be very portable. >> >> Benchmark: mean over 3 executions of >> ELM_TEST_AUTOBOUNCE=100 time elementary_test -to genlist >> >> ``` >> BEFORE AFTER >> ------------------------------------------------------------ >> time (ns) 11114111647.0 9147676220.0 >> frames 2872.3333333333335 2904.6666666666665 >> time per frame (ns) 3869364.6666666665 3149535.3333333335 >> user time (s) 11.096666666666666 9.22 >> cpu (%) 22.666666666666668 18.333333333333332 >> ``` >> >> Ref T6580 >> >> Reviewers: raster, cedric >> >> Subscribers: cedric, jpeg >> >> Maniphest Tasks: T6580 >> >> Differential Revision: https://phab.enlightenment.org/D5738 >> --- >> src/lib/ecore_evas/ecore_evas_private.h | 2 +- >> src/lib/eina/eina_cow.c | 1 + >> src/lib/eina/eina_safepointer.c | 2 +- >> src/lib/eina/eina_types.h | 8 +++++ >> src/lib/eo/Eo.h | 56 >> +++++++---------------------- >> src/lib/eo/eo.c | 64 >> ++++++--------------------------- >> 6 files changed, 33 insertions(+), 100 deletions(-) >> >> diff --git a/src/lib/ecore_evas/ecore_evas_private.h >> b/src/lib/ecore_evas/ecore_evas_private.h >> index 7b83589347..13002402a4 100644 >> --- a/src/lib/ecore_evas/ecore_evas_private.h >> +++ b/src/lib/ecore_evas/ecore_evas_private.h >> @@ -347,7 +347,7 @@ struct _Ecore_Evas >> } delayed; >> >> int refcount; >> -#define ECORE_EVAS_ASYNC_RENDER_DEBUG 1 /* TODO: remove me */ >> +//#define ECORE_EVAS_ASYNC_RENDER_DEBUG 1 /* TODO: remove me */ >> #ifdef ECORE_EVAS_ASYNC_RENDER_DEBUG >> double async_render_start; >> #endif >> diff --git a/src/lib/eina/eina_cow.c b/src/lib/eina/eina_cow.c >> index 73a65e5f30..8ec86cfd79 100644 >> --- a/src/lib/eina/eina_cow.c >> +++ b/src/lib/eina/eina_cow.c >> @@ -320,6 +320,7 @@ eina_cow_init(void) >> Eina_Bool >> eina_cow_shutdown(void) >> { >> + eina_log_domain_unregister(_eina_cow_log_dom); >> eina_mempool_del(gc_pool); >> return EINA_TRUE; >> } >> diff --git a/src/lib/eina/eina_safepointer.c >> b/src/lib/eina/eina_safepointer.c >> index 1a19f851c7..8f9d4b62a9 100644 >> --- a/src/lib/eina/eina_safepointer.c >> +++ b/src/lib/eina/eina_safepointer.c >> @@ -370,7 +370,7 @@ eina_safepointer_init(void) >> >> DBG("entry[Size, Align] = { %zu, %u }", >> sizeof (Eina_Memory_Entry), eina_mempool_alignof(sizeof >> (Eina_Memory_Entry))); >> - DBG("table[Size, Align] = { %zu, %u }\n", >> + DBG("table[Size, Align] = { %zu, %u }", >> sizeof (Eina_Memory_Table), eina_mempool_alignof(sizeof >> (Eina_Memory_Table))); >> >> return EINA_TRUE; >> diff --git a/src/lib/eina/eina_types.h b/src/lib/eina/eina_types.h >> index 94e6ebc6f3..3d15a3f42d 100644 >> --- a/src/lib/eina/eina_types.h >> +++ b/src/lib/eina/eina_types.h >> @@ -184,6 +184,14 @@ >> # define EINA_PURE >> # endif >> >> +# if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8) >> +# define EINA_HOT __attribute__ ((hot)) >> +# define EINA_COLD __attribute__ ((cold)) >> +# else >> +# define EINA_HOT >> +# define EINA_COLD >> +# endif >> + >> # if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4) >> # if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 3) >> # define EINA_PRINTF(fmt, arg) __attribute__((__format__ >> (__gnu_printf__, fmt, arg))) >> diff --git a/src/lib/eo/Eo.h b/src/lib/eo/Eo.h >> index 3ea20ec2ff..8b489ab72a 100644 >> --- a/src/lib/eo/Eo.h >> +++ b/src/lib/eo/Eo.h >> @@ -1159,37 +1159,6 @@ typedef struct _Efl_Object_Op_Call_Data >> void *extn4; // for future use to avoid ABI issues >> } Efl_Object_Op_Call_Data; >> >> -#define EFL_OBJECT_CALL_CACHE_SIZE 1 >> - >> -typedef struct _Efl_Object_Call_Cache_Index >> -{ >> - const void *klass; >> -} Efl_Object_Call_Cache_Index; >> - >> -typedef struct _Efl_Object_Call_Cache_Entry >> -{ >> - const void *func; >> -} Efl_Object_Call_Cache_Entry; >> - >> -typedef struct _Efl_Object_Call_Cache_Off >> -{ >> - int off; >> -} Efl_Object_Call_Cache_Off; >> - >> -typedef struct _Efl_Object_Call_Cache >> -{ >> -#if EFL_OBJECT_CALL_CACHE_SIZE > 0 >> - Efl_Object_Call_Cache_Index index[EFL_OBJECT_CALL_CACHE_SIZE]; >> - Efl_Object_Call_Cache_Entry entry[EFL_OBJECT_CALL_CACHE_SIZE]; >> - Efl_Object_Call_Cache_Off off [EFL_OBJECT_CALL_CACHE_SIZE]; >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 1 >> - int next_slot; >> -# endif >> -#endif >> - Efl_Object_Op op; >> - unsigned int generation; >> -} Efl_Object_Call_Cache; >> - >> // to pass the internal function call to EFL_FUNC_BODY (as Func >> parameter) >> #define EFL_FUNC_CALL(...) __VA_ARGS__ >> >> @@ -1205,17 +1174,18 @@ typedef struct _Efl_Object_Call_Cache >> # define EFL_FUNC_TLS __thread >> #endif >> >> + >> // cache OP id, get real fct and object data then do the call >> #define EFL_FUNC_COMMON_OP(Obj, Name, DefRet) \ >> - static EFL_FUNC_TLS Efl_Object_Call_Cache ___cache; /* static 0 by >> default */ \ >> + static Efl_Object_Op ___op; /* static 0 by default */ \ >> Efl_Object_Op_Call_Data ___call; \ >> _Eo_##Name##_func _func_; \ >> - if (EINA_UNLIKELY((___cache.op == EFL_NOOP) || \ >> - (___cache.generation != >> _efl_object_init_generation))) \ >> + if (EINA_UNLIKELY(___op == EFL_NOOP)) \ >> goto __##Name##_op_create; /* yes a goto - see below */ \ >> - __##Name##_op_create_done: \ >> - if (!_efl_object_call_resolve((Eo *) Obj, #Name, &___call, &___cache, >> \ >> - __FILE__, __LINE__)) goto >> __##Name##_failed; \ >> + __##Name##_op_create_done: EINA_HOT; \ >> + if (EINA_UNLIKELY(!_efl_object_call_resolve( \ >> + (Eo *) Obj, #Name, &___call, ___op, __FILE__, __LINE__))) \ >> + goto __##Name##_failed; \ >> _func_ = (_Eo_##Name##_func) ___call.func; >> >> // This looks ugly with gotos BUT it moves rare "init" handling code >> @@ -1227,13 +1197,11 @@ typedef struct _Efl_Object_Call_Cache >> // of the cacheline that was already fetched should yield better cache >> // hits. >> #define EFL_FUNC_COMMON_OP_END(Obj, Name, DefRet, ErrorCase) \ >> -__##Name##_op_create: \ >> - if (EINA_UNLIKELY(___cache.op != EFL_NOOP)) memset(&___cache, 0, >> sizeof(___cache)); \ >> - ___cache.op = _efl_object_op_api_id_get(EFL_FUNC_COMMON_OP_FUNC(Name), >> Obj, #Name, __FILE__, __LINE__); \ >> - if (___cache.op == EFL_NOOP) goto __##Name##_failed; \ >> - ___cache.generation = _efl_object_init_generation; \ >> +__##Name##_op_create: EINA_COLD; \ >> + ___op = _efl_object_op_api_id_get(EFL_FUNC_COMMON_OP_FUNC(Name), >> Obj, #Name, __FILE__, __LINE__); \ >> + if (EINA_UNLIKELY(___op == EFL_NOOP)) goto __##Name##_failed; \ >> goto __##Name##_op_create_done; \ >> -__##Name##_failed: \ >> +__##Name##_failed: EINA_COLD; \ >> ErrorCase \ >> return DefRet; >> #define _EFL_OBJECT_API_BEFORE_HOOK >> @@ -1335,7 +1303,7 @@ EAPI Efl_Object_Op _efl_object_api_op_id_get(const >> void *api_func) EINA_DEPRECAT >> 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) >> EINA_ARG_NONNULL(1, 2, 3, 4) EINA_WARN_UNUSED_RESULT; >> >> // gets the real function pointer and the object data >> -EAPI Eina_Bool _efl_object_call_resolve(Eo *obj, const char *func_name, >> Efl_Object_Op_Call_Data *call, Efl_Object_Call_Cache *callcache, const char >> *file, int line); >> +EAPI Eina_Bool _efl_object_call_resolve(Eo *obj, const char *func_name, >> Efl_Object_Op_Call_Data *call, Efl_Object_Op op, const char *file, int >> line); >> >> // end of the eo call barrier, unref the obj >> EAPI void _efl_object_call_end(Efl_Object_Op_Call_Data *call); >> diff --git a/src/lib/eo/eo.c b/src/lib/eo/eo.c >> index 50df250acf..0db34fc35c 100644 >> --- a/src/lib/eo/eo.c >> +++ b/src/lib/eo/eo.c >> @@ -439,15 +439,14 @@ efl_cast(const Eo *eo_id, const Efl_Class >> *cur_klass) >> } >> >> EAPI Eina_Bool >> -_efl_object_call_resolve(Eo *eo_id, const char *func_name, >> Efl_Object_Op_Call_Data *call, Efl_Object_Call_Cache *cache, const char >> *file, int line) >> +_efl_object_call_resolve(Eo *eo_id, const char *func_name, >> Efl_Object_Op_Call_Data *call, Efl_Object_Op op, const char *file, int line) >> { >> - const _Efl_Class *klass, *inputklass, *main_klass; >> + const _Efl_Class *klass, *main_klass; >> const _Efl_Class *cur_klass = NULL; >> _Eo_Object *obj = NULL; >> const Eo_Vtable *vtable = NULL; >> const op_type_funcs *func; >> Eina_Bool is_obj; >> - Eina_Bool is_override = EINA_FALSE; >> Eina_Bool super = EINA_TRUE; >> >> if (EINA_UNLIKELY(!eo_id)) return EINA_FALSE; >> @@ -456,14 +455,13 @@ _efl_object_call_resolve(Eo *eo_id, const char >> *func_name, Efl_Object_Op_Call_Da >> >> is_obj = _eo_is_a_obj(eo_id); >> >> - if (is_obj) >> + if (EINA_LIKELY(is_obj == EINA_TRUE)) >> { >> EO_OBJ_POINTER_RETURN_VAL_PROXY(eo_id, _obj, EINA_FALSE); >> >> obj = _obj; >> klass = _obj->klass; >> vtable = EO_VTABLE(obj); >> - is_override = _obj_is_override(obj); >> if (EINA_UNLIKELY(_obj->cur_klass != NULL)) >> { >> // YES this is a goto with a label to return. this is a >> @@ -485,9 +483,7 @@ obj_super_back: >> } >> ok_klass_back: >> >> - inputklass = main_klass = klass; >> - >> - if (!cache->op) goto err_cache_op; >> + main_klass = klass; >> >> /* If we have a current class, we need to itr to the next. */ >> if (cur_klass) >> @@ -499,30 +495,7 @@ ok_klass_back: >> } >> else >> { >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 0 >> - if (!is_override) >> - { >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 1 >> - int i; >> - >> - for (i = 0; i < EFL_OBJECT_CALL_CACHE_SIZE; i++) >> -# else >> - const int i = 0; >> -# endif >> - { >> - if ((const void *)inputklass == cache->index[i].klass) >> - { >> - func = (const op_type_funcs >> *)cache->entry[i].func; >> - call->func = func->func; >> - if (is_obj) >> - call->data = (char *)obj + cache->off[i].off; >> - if (EINA_UNLIKELY(!call->func)) goto err_cache_op; >> - return EINA_TRUE; >> - } >> - } >> - } >> -#endif >> - func = _vtable_func_get(vtable, cache->op); >> + func = _vtable_func_get(vtable, op); >> // this is not very likely to happen - but may if its an invalid >> // call or a composite object, but either way, it's not very >> likely >> // so make it a goto to save on instruction cache >> @@ -536,22 +509,6 @@ ok_cur_klass_back: >> >> if (is_obj) call->data = _efl_data_scope_get(obj, func->src); >> >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 0 >> - if (!cur_klass && !is_override) >> - { >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 1 >> - const int slot = cache->next_slot; >> -# else >> - const int slot = 0; >> -# endif >> - cache->index[slot].klass = (const void *)inputklass; >> - cache->entry[slot].func = (const void *)func; >> - cache->off[slot].off = (int)((long)((char *)call->data - >> (char *)obj)); >> -# if EFL_OBJECT_CALL_CACHE_SIZE > 1 >> - cache->next_slot = (slot + 1) % EFL_OBJECT_CALL_CACHE_SIZE; >> -# endif >> - } >> -#endif >> return EINA_TRUE; >> } >> >> @@ -570,7 +527,7 @@ end: >> EO_OBJ_POINTER_PROXY(emb_obj_id, emb_obj); >> if (EINA_UNLIKELY(!emb_obj)) continue; >> >> - func = _vtable_func_get(EO_VTABLE(emb_obj), cache->op); >> + func = _vtable_func_get(EO_VTABLE(emb_obj), op); >> if (func == NULL) goto composite_continue; >> >> if (EINA_LIKELY(func->func && func->src)) >> @@ -594,7 +551,7 @@ composite_continue: >> if (cur_klass) >> { >> ERR("in %s:%d: func '%s' (%d) could not be resolved for class >> '%s' for super of '%s'.", >> - file, line, func_name, cache->op, main_klass->desc->name, >> + file, line, func_name, op, main_klass->desc->name, >> cur_klass->desc->name); >> goto err; >> } >> @@ -602,7 +559,7 @@ composite_continue: >> { >> /* we should not be able to take this branch */ >> ERR("in %s:%d: func '%s' (%d) could not be resolved for class >> '%s'.", >> - file, line, func_name, cache->op, main_klass->desc->name); >> + file, line, func_name, op, main_klass->desc->name); >> goto err; >> } >> err_cache_op: >> @@ -612,7 +569,7 @@ err_cache_op: >> goto err; >> err_func_src: >> ERR("in %s:%d: you called a pure virtual func '%s' (%d) of class >> '%s'.", >> - file, line, func_name, cache->op, klass->desc->name); >> + file, line, func_name, op, klass->desc->name); >> err: >> if (is_obj) >> { >> @@ -629,7 +586,7 @@ err: >> // yes - special "move out of hot path" code blobs with goto's for >> // speed reasons to have intr prefetches work better and miss less >> ok_cur_klass: >> - func = _eo_kls_itr_next(klass, cur_klass, cache->op, super); >> + func = _eo_kls_itr_next(klass, cur_klass, op, super); >> if (!func) goto end; >> klass = func->src; >> goto ok_cur_klass_back; >> @@ -661,7 +618,6 @@ obj_super: >> cur_klass = NULL; >> } >> >> - is_override = _obj_is_override(obj) && (cur_klass == NULL); >> } >> goto obj_super_back; >> >> >> -- >> >> -- >> Jean-Philippe André >> >> -- Jean-Philippe André ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
