Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-20 Thread The Rasterman 
On Tue, 20 Sep 2016 00:56:24 -0300 Gustavo Sverzut Barbieri
 said:

> On Mon, Sep 19, 2016 at 8:08 PM, Carsten Haitzler 
> wrote:
> > On Mon, 19 Sep 2016 09:04:15 -0300 Gustavo Sverzut Barbieri
> >  said:
> >
> >> On Sun, Sep 18, 2016 at 2:03 PM, Carsten Haitzler 
> >> wrote:
> >> > On Thu, 15 Sep 2016 08:00:46 -0300 Gustavo Sverzut Barbieri
> >> >  said:
> >> >
> >> >> isn't it better to check in ecore_thread_cancel() if the thread was
> >> >> already created?
> >> >
> >> > that'd require keeping track of every thread we create, looking up in
> >> > that list/table ever time etc. ... like eoid does. eina_thread stuff is
> >> > are ally a tiny tiny tiny thin wrapper over posix (or windows or osx)
> >> > threading - same with locks/semaphores etc. at this level such a complex
> >> > check isn't really worth it.
> >>
> >> there is a single place inside ecore_thread_cancel() that calls this
> >> function and it knows if t->self is 0 before calling.
> >
> > ummm... you're mixing ecore_thread and eina_thread. different things. :)
> > this here is about eina_thread which has no t->self. t (thread) *IS*
> > pthread_t. it's opaque. well it hapens to be pthread_t but we could change
> > it and alloc a struct in future as long as abi remains the same.
> >
> > so how do we check if t (Eina_Thread) has been created. basically we'd need
> > to
> >
> > if (eina_list_find(all_eina_threads, t))
> >   return pthread_cancel((pthread_t)t) == 0;
> > return EINA_FALSE;
> >
> > that means a list walk every time to go find the handle. every
> > eina_thread_create() has to:
> >
> > all_eina_threads = eina_list_append(all_eina_threads, t);
> >
> > We could use a hash. an array. point is we don't track all threads. we'd
> > have to start tracking them and to do a check would be a lookup like above.
> 
> we're talking about different things for different reasons :-D

i'm JUST talking eina_thread_cancel(). nothing else :)

> I've added eina_thread_cancel() just to not expose pthread_cancel().
> Most of users should never ever call that directly as they will do
> mistakes with the threads. pthread_cancel() should return ESRCH if the
> thread couldn't be found... not SEGV. And it's doing that for other
> stuff, like pthread_join()... which we simply call without doing any
> check on given Eina_Thread (thus would crash as well).
> 
> Users should use Ecore_Thread or something we may put on top (like
> some Eo object). Ecore_Thread always provided ecore_thread_cancel(),
> that only queues a request so users may ecore_thread_check() and stop
> working.
> 
> With my change, ecore_thread_cancel() will call eina_thread_cancel(),
> usually that's innocuous as thread cancellation is disabled. If the
> user choose to enable thread cancellation, that is going to cancel the
> thread preemptively at some well known points (man:pthreads(7)).
> 
> THUS, ecore_thread_cancel() does know if it create  the thread, if the
> thread was already joined, etc. It can easily check and do not call
> eina_thread_cancel().
> 
> Alternatively we may EINA_SAFETY_CHECK all entry point of
> eina_thread.h, but I don't think we should do that given how low level
> it is.
> 
> 
> 
> -- 
> Gustavo Sverzut Barbieri
> --
> Mobile: +55 (16) 99354-9890
> 


-- 
- 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

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-19 Thread Gustavo Sverzut Barbieri 
On Mon, Sep 19, 2016 at 8:08 PM, Carsten Haitzler  wrote:
> On Mon, 19 Sep 2016 09:04:15 -0300 Gustavo Sverzut Barbieri
>  said:
>
>> On Sun, Sep 18, 2016 at 2:03 PM, Carsten Haitzler 
>> wrote:
>> > On Thu, 15 Sep 2016 08:00:46 -0300 Gustavo Sverzut Barbieri
>> >  said:
>> >
>> >> isn't it better to check in ecore_thread_cancel() if the thread was
>> >> already created?
>> >
>> > that'd require keeping track of every thread we create, looking up in that
>> > list/table ever time etc. ... like eoid does. eina_thread stuff is are ally
>> > a tiny tiny tiny thin wrapper over posix (or windows or osx) threading -
>> > same with locks/semaphores etc. at this level such a complex check isn't
>> > really worth it.
>>
>> there is a single place inside ecore_thread_cancel() that calls this
>> function and it knows if t->self is 0 before calling.
>
> ummm... you're mixing ecore_thread and eina_thread. different things. :) this
> here is about eina_thread which has no t->self. t (thread) *IS* pthread_t. 
> it's
> opaque. well it hapens to be pthread_t but we could change it and alloc a
> struct in future as long as abi remains the same.
>
> so how do we check if t (Eina_Thread) has been created. basically we'd need to
>
> if (eina_list_find(all_eina_threads, t))
>   return pthread_cancel((pthread_t)t) == 0;
> return EINA_FALSE;
>
> that means a list walk every time to go find the handle. every
> eina_thread_create() has to:
>
> all_eina_threads = eina_list_append(all_eina_threads, t);
>
> We could use a hash. an array. point is we don't track all threads. we'd  have
> to start tracking them and to do a check would be a lookup like above.

we're talking about different things for different reasons :-D

I've added eina_thread_cancel() just to not expose pthread_cancel().
Most of users should never ever call that directly as they will do
mistakes with the threads. pthread_cancel() should return ESRCH if the
thread couldn't be found... not SEGV. And it's doing that for other
stuff, like pthread_join()... which we simply call without doing any
check on given Eina_Thread (thus would crash as well).

Users should use Ecore_Thread or something we may put on top (like
some Eo object). Ecore_Thread always provided ecore_thread_cancel(),
that only queues a request so users may ecore_thread_check() and stop
working.

With my change, ecore_thread_cancel() will call eina_thread_cancel(),
usually that's innocuous as thread cancellation is disabled. If the
user choose to enable thread cancellation, that is going to cancel the
thread preemptively at some well known points (man:pthreads(7)).

THUS, ecore_thread_cancel() does know if it create  the thread, if the
thread was already joined, etc. It can easily check and do not call
eina_thread_cancel().

Alternatively we may EINA_SAFETY_CHECK all entry point of
eina_thread.h, but I don't think we should do that given how low level
it is.



-- 
Gustavo Sverzut Barbieri
--
Mobile: +55 (16) 99354-9890

--
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-19 Thread The Rasterman 
On Mon, 19 Sep 2016 09:04:15 -0300 Gustavo Sverzut Barbieri
 said:

> On Sun, Sep 18, 2016 at 2:03 PM, Carsten Haitzler 
> wrote:
> > On Thu, 15 Sep 2016 08:00:46 -0300 Gustavo Sverzut Barbieri
> >  said:
> >
> >> isn't it better to check in ecore_thread_cancel() if the thread was
> >> already created?
> >
> > that'd require keeping track of every thread we create, looking up in that
> > list/table ever time etc. ... like eoid does. eina_thread stuff is are ally
> > a tiny tiny tiny thin wrapper over posix (or windows or osx) threading -
> > same with locks/semaphores etc. at this level such a complex check isn't
> > really worth it.
> 
> there is a single place inside ecore_thread_cancel() that calls this
> function and it knows if t->self is 0 before calling.

ummm... you're mixing ecore_thread and eina_thread. different things. :) this
here is about eina_thread which has no t->self. t (thread) *IS* pthread_t. it's
opaque. well it hapens to be pthread_t but we could change it and alloc a
struct in future as long as abi remains the same.

so how do we check if t (Eina_Thread) has been created. basically we'd need to

if (eina_list_find(all_eina_threads, t))
  return pthread_cancel((pthread_t)t) == 0;
return EINA_FALSE;

that means a list walk every time to go find the handle. every
eina_thread_create() has to:

all_eina_threads = eina_list_append(all_eina_threads, t);

We could use a hash. an array. point is we don't track all threads. we'd  have
to start tracking them and to do a check would be a lookup like above.

-- 
- 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

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-19 Thread Gustavo Sverzut Barbieri 
On Sun, Sep 18, 2016 at 2:03 PM, Carsten Haitzler  wrote:
> On Thu, 15 Sep 2016 08:00:46 -0300 Gustavo Sverzut Barbieri
>  said:
>
>> isn't it better to check in ecore_thread_cancel() if the thread was
>> already created?
>
> that'd require keeping track of every thread we create, looking up in that
> list/table ever time etc. ... like eoid does. eina_thread stuff is are ally a
> tiny tiny tiny thin wrapper over posix (or windows or osx) threading - same
> with locks/semaphores etc. at this level such a complex check isn't really 
> worth
> it.

there is a single place inside ecore_thread_cancel() that calls this
function and it knows if t->self is 0 before calling.




-- 
Gustavo Sverzut Barbieri
--
Mobile: +55 (16) 99354-9890

--
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-18 Thread The Rasterman 
On Thu, 15 Sep 2016 08:00:46 -0300 Gustavo Sverzut Barbieri
 said:

> isn't it better to check in ecore_thread_cancel() if the thread was
> already created?

that'd require keeping track of every thread we create, looking up in that
list/table ever time etc. ... like eoid does. eina_thread stuff is are ally a
tiny tiny tiny thin wrapper over posix (or windows or osx) threading - same
with locks/semaphores etc. at this level such a complex check isn't really worth
it.

> On Wed, Sep 14, 2016 at 8:52 PM, Cedric BAIL  wrote:
> > cedric pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=4d69f472fe31a9006436a05282a8376c7a712d41
> >
> > commit 4d69f472fe31a9006436a05282a8376c7a712d41
> > Author: Cedric Bail 
> > Date:   Wed Sep 14 16:50:05 2016 -0700
> >
> > eina: allow graceful failure when calling eina_thread_cancel with NULL.
> > ---
> >  src/lib/eina/eina_thread.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/lib/eina/eina_thread.c b/src/lib/eina/eina_thread.c
> > index d40073a..5002a42 100644
> > --- a/src/lib/eina/eina_thread.c
> > +++ b/src/lib/eina/eina_thread.c
> > @@ -230,6 +230,7 @@ eina_thread_name_set(Eina_Thread t, const char *name)
> >  EAPI Eina_Bool
> >  eina_thread_cancel(Eina_Thread t)
> >  {
> > +   if (!t) return EINA_FALSE;
> > return pthread_cancel((pthread_t)t) == 0;
> >  }
> >
> >
> > --
> >
> >
> 
> 
> 
> -- 
> Gustavo Sverzut Barbieri
> --
> Mobile: +55 (16) 99354-9890
> 
> --
> ___
> 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

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-18 Thread The Rasterman 
On Thu, 15 Sep 2016 11:31:01 -0700 Cedric BAIL  said:

> On Thu, Sep 15, 2016 at 4:00 AM, Gustavo Sverzut Barbieri
>  wrote:
> > isn't it better to check in ecore_thread_cancel() if the thread was
> > already created?
> 
> I was wondering also, but settled for this as it will protect any
> caller and the code is costly anyway, so no performance impact.

actually i'd like to nip this in the bud here. it is not zero performance
impact. that if and all the code inside the if's {} section are a cost. cost to
code size. a cost to l1 cache for the INSTRUCTION cash (and cacheline prefetch).

in fact i recently doubled the speed of eoid lookups JUST by removing if's and
mostly their CONTENT because the content of "rare" error handling was more than
just a return ... i moved the error handling to the function end with goto's
and got a 2x speedup.

in fact i want to do a general writeup on this... but this is very important.
all out "if's" handling error input, printing some log etc. when such errors are
the rare case, not common... we're losing a LOT of perf to l1 cache misses
where l1 cachelines prefetched code we won't run. EINA_LIKELY/UNLIKELY doesn't
help at all. this actually hasn't helped since 486 days.

we really need to change our style. no longer:

void func(void *p, int x, int y) {
  int z;
  if (!p) {
ERR("some log is NULL");
return;
  }
  if (x < 1) {
ERR("doing %p, x < 1 blah\n", p);
return;
  }
  if (y < 1) {
ERR("doing %p, y < 1 blah\n", p);
return;
  }
  // actual code here
  z = x + y;
  ...
}

literally the above has a TONNE of cache misses and it HURTS... 

i'll repeat...

EOID LOOKUP was 2 TIIMES FASTER. in fact it wen fro ~5-7% CPU down to STEADY
2.5% CPU. all to do with l1 *INSTRUCTION* cache hits vs misses...

i'm doing an informal call here - all code like the above should change to
something like:

void func(void *p, int x, int y) {
  int z;
  if (!p) goto err_badp;
  if (x < 1) goto err_badx;
  if (y < 1) goto err_bady;
  // actual code here
  z = x + y;
  ...
  return;
// "rare case" error handling here
err_badp:
  ERR("some log is NULL");
  return;
err_badx:
  ERR("doing %p, x < 1 blah\n", p);
  return;
err_bady:
  ERR("doing %p, y < 1 blah\n", p);
  return;
}

yes. you could merge goto's with if ((!p) || (x < 1) || (y < 1)) etc. BUT move
the error handling code out of the way of the linear read-ahead by the l1 cache.
remember it's reading a cacheline at a time so pre-fetching future
instructions. the compiler IS putting the code inside the if {} right there in
the assembly where it is in c. it doesn't re-order it. yes. i stumbled upon what
is possibly a major compiler optimization opportunity. buty until such a
magical day comes... we need to re-do our code so we move "rarely used code
paths" out of the "linear codepath" where the cpu is fetching another cacheline
then the next etc. but then skipping what it fetched only to get a cache miss
and thus has to stop and wait. it was utterly amazing just how much this helps
on hot code paths and frankly it doesn't make the code really any worse. it's
nicer to read as the error handling is out of the way of scanning the code
ahead (or rarely executed code is). (ok ok we could re-order code too to have
success case inside if {}'s but that would be far harder in many cases to do
than a goto).

GOTO IS AWESOME. 'nuff said.

it's a really MEASURABLE difference in speed. consistent. more than a double
speedup. between 2 to 3 TIMES faster for UNCACHED code (and that is most of our
code except inside inner loops, though even with inner loops it would help
moving rare cases out of the inner loop code so we can fit more code in l1
cache).

seriously consider this.

these if's are NOT FREE. yes - this one is just a "return" so it'd be the same
as a goto ... but if you just added a printf or an eina log etc. it'd blow out
in cost FAST.

in fact a lot of these static inline funcs in eina are polluting l1 cache badly
by littering our code with the error handling code. we need to revise this.
it's costly. it's not free.

> > On Wed, Sep 14, 2016 at 8:52 PM, Cedric BAIL  wrote:
> >> cedric pushed a commit to branch master.
> >>
> >> http://git.enlightenment.org/core/efl.git/commit/?id=4d69f472fe31a9006436a05282a8376c7a712d41
> >>
> >> commit 4d69f472fe31a9006436a05282a8376c7a712d41
> >> Author: Cedric Bail 
> >> Date:   Wed Sep 14 16:50:05 2016 -0700
> >>
> >> eina: allow graceful failure when calling eina_thread_cancel with NULL.
> >> ---
> >>  src/lib/eina/eina_thread.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/lib/eina/eina_thread.c b/src/lib/eina/eina_thread.c
> >> index d40073a..5002a42 100644
> >> --- a/src/lib/eina/eina_thread.c
> >> +++ b/src/lib/eina/eina_thread.c
> >> @@ -230,6 +230,7 @@ eina_thread_name_set(Eina_Thread t, const char *name)
> >>  EAPI Eina_Bool
> >>  eina_thread_cancel(Eina_Thread t)
> >>

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-15 Thread Cedric BAIL 
On Thu, Sep 15, 2016 at 4:00 AM, Gustavo Sverzut Barbieri
 wrote:
> isn't it better to check in ecore_thread_cancel() if the thread was
> already created?

I was wondering also, but settled for this as it will protect any
caller and the code is costly anyway, so no performance impact.

> On Wed, Sep 14, 2016 at 8:52 PM, Cedric BAIL  wrote:
>> cedric pushed a commit to branch master.
>>
>> http://git.enlightenment.org/core/efl.git/commit/?id=4d69f472fe31a9006436a05282a8376c7a712d41
>>
>> commit 4d69f472fe31a9006436a05282a8376c7a712d41
>> Author: Cedric Bail 
>> Date:   Wed Sep 14 16:50:05 2016 -0700
>>
>> eina: allow graceful failure when calling eina_thread_cancel with NULL.
>> ---
>>  src/lib/eina/eina_thread.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/lib/eina/eina_thread.c b/src/lib/eina/eina_thread.c
>> index d40073a..5002a42 100644
>> --- a/src/lib/eina/eina_thread.c
>> +++ b/src/lib/eina/eina_thread.c
>> @@ -230,6 +230,7 @@ eina_thread_name_set(Eina_Thread t, const char *name)
>>  EAPI Eina_Bool
>>  eina_thread_cancel(Eina_Thread t)
>>  {
>> +   if (!t) return EINA_FALSE;
>> return pthread_cancel((pthread_t)t) == 0;
>>  }
>>
>>
>> --
>>
>>
>
>
>
> --
> Gustavo Sverzut Barbieri
> --
> Mobile: +55 (16) 99354-9890
>
> --
> ___
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel



-- 
Cedric BAIL

--
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Re: [E-devel] [EGIT] [core/efl] master 01/02: eina: allow graceful failure when calling eina_thread_cancel with NULL.

2016-09-15 Thread Gustavo Sverzut Barbieri 
isn't it better to check in ecore_thread_cancel() if the thread was
already created?

On Wed, Sep 14, 2016 at 8:52 PM, Cedric BAIL  wrote:
> cedric pushed a commit to branch master.
>
> http://git.enlightenment.org/core/efl.git/commit/?id=4d69f472fe31a9006436a05282a8376c7a712d41
>
> commit 4d69f472fe31a9006436a05282a8376c7a712d41
> Author: Cedric Bail 
> Date:   Wed Sep 14 16:50:05 2016 -0700
>
> eina: allow graceful failure when calling eina_thread_cancel with NULL.
> ---
>  src/lib/eina/eina_thread.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/lib/eina/eina_thread.c b/src/lib/eina/eina_thread.c
> index d40073a..5002a42 100644
> --- a/src/lib/eina/eina_thread.c
> +++ b/src/lib/eina/eina_thread.c
> @@ -230,6 +230,7 @@ eina_thread_name_set(Eina_Thread t, const char *name)
>  EAPI Eina_Bool
>  eina_thread_cancel(Eina_Thread t)
>  {
> +   if (!t) return EINA_FALSE;
> return pthread_cancel((pthread_t)t) == 0;
>  }
>
>
> --
>
>



-- 
Gustavo Sverzut Barbieri
--
Mobile: +55 (16) 99354-9890

--
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel