On Fri, 2 Sep 2016 15:15:07 +0100 Tom Hacohen <t...@osg.samsung.com> said:
> Good job trying to tackle it, but this is wrong. > > This is not thread-safe and the Eo infrastructure should be. So > essentially, this optimisation is not allowed. You could maybe do a > cache per-thread, raster claims that __thread is actually quite alright > nowadays with GCC, I'm not sure, but this, and the following patch > should be reverted. i looked more and more. we are both right. and both wrong. __thread is FREE ... in a binary. it's a tls lookup in a .so. because the compiler cant pre-allocate a memory region for this info so it has to use the current stack frame to jump to a tls section. > If you have anything to say to defend this approach, please do so by > Monday, otherwise I'll revert both patches then. ummm i already wrapped them with spinlocks earlier this week. thats when i started to go "there has to be a better way" because real life testing (perf not callgrind) made eo ptr lookup go from 2% to 5% of cpu. thus my wondering if making this per-thread might not help. that is a whole topic on its own BUT i still think its a good idea: tls 0.33556 400000002 = 1.546 ns / get spin 0.89498 300000000 = 7.585 ns / lock+release a tls lookup is almost 5 TIMES faster than a spinlock lock + unlock. given that and our discussion which i'll do a writeup on that adds safety to eo and threading (ie makes it impossible to use an eo obj outside its owning thread unless its explicitly sent/shared there) is probably totally worth it as we can remove basically all the locks really used outside of app init (_eoid_lock, an eo_isa lock, klass->iterators.trash_lock). but this does bring design considerations into play and we need to have a talk about this. i'll draft up some mail describing this all later. > -- > Tom. > > On 26/08/16 20:14, Cedric BAIL wrote: > > cedric pushed a commit to branch master. > > > > http://git.enlightenment.org/core/efl.git/commit/?id=93a706a947cd2d6ef80f8704f4e23737fea1258f > > > > commit 93a706a947cd2d6ef80f8704f4e23737fea1258f > > Author: Cedric BAIL <ced...@osg.samsung.com> > > Date: Fri Aug 26 12:03:08 2016 -0700 > > > > eo: general speedup of all Eo related operation. > > > > This change rely on the fact that we do fetch the same > > object id over and over again. _efl_object_call_resolve got > > 15% faster, efl_data_scope_get 20%. > > --- > > src/lib/eo/eo_base_class.c | 3 +++ > > src/lib/eo/eo_ptr_indirection.x | 20 +++++++++++++++++++- > > 2 files changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/src/lib/eo/eo_base_class.c b/src/lib/eo/eo_base_class.c > > index 732d063..35c7a9c 100644 > > --- a/src/lib/eo/eo_base_class.c > > +++ b/src/lib/eo/eo_base_class.c > > @@ -11,6 +11,9 @@ > > > > static int event_freeze_count = 0; > > > > +_Eo_Object *cached_object = NULL; > > +Eo_Id cached_id = 0; > > + > > typedef struct _Eo_Callback_Description Eo_Callback_Description; > > > > typedef struct > > diff --git a/src/lib/eo/eo_ptr_indirection.x > > b/src/lib/eo/eo_ptr_indirection.x index d89b1ec..cae59a4 100644 > > --- a/src/lib/eo/eo_ptr_indirection.x > > +++ b/src/lib/eo/eo_ptr_indirection.x > > @@ -269,6 +269,9 @@ extern Generation_Counter _eo_generation_counter; > > /* Macro used for readability */ > > #define TABLE_FROM_IDS _eo_ids_tables[mid_table_id][table_id] > > > > +extern _Eo_Object *cached_object; > > +extern Eo_Id cached_id; > > + > > static inline _Eo_Object * > > _eo_obj_pointer_get(const Eo_Id obj_id) > > { > > @@ -289,6 +292,10 @@ _eo_obj_pointer_get(const Eo_Id obj_id) > > DBG("obj_id is not a valid object id."); > > return NULL; > > } > > + else if (obj_id == cached_id) > > + { > > + return cached_object; > > + } > > > > EO_DECOMPOSE_ID(obj_id, mid_table_id, table_id, entry_id, generation); > > > > @@ -297,7 +304,13 @@ _eo_obj_pointer_get(const Eo_Id obj_id) > > { > > entry = &(TABLE_FROM_IDS->entries[entry_id]); > > if (entry && entry->active && (entry->generation == generation)) > > - return entry->ptr; > > + { > > + // Cache the result of that lookup > > + cached_object = entry->ptr; > > + cached_id = obj_id; > > + > > + return entry->ptr; > > + } > > } > > > > ERR("obj_id %p is not pointing to a valid object. Maybe it has already > > been freed.", @@ -477,6 +490,11 @@ _eo_id_release(const Eo_Id obj_id) > > if (_current_table == table) > > _current_table = NULL; > > } > > + > > + // In case an object is destroyed, wipe out the cache > > + cached_id = 0; > > + cached_object = NULL; > > + > > return; > > } > > } > > > > > ------------------------------------------------------------------------------ > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
