On Thu, 15 Sep 2016 11:31:01 -0700 Cedric BAIL <cedric.b...@free.fr> said:

> On Thu, Sep 15, 2016 at 4:00 AM, Gustavo Sverzut Barbieri
> <barbi...@gmail.com> 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 <cedric.b...@free.fr> wrote:
> >> cedric pushed a commit to branch master.
> >>
> >> http://git.enlightenment.org/core/efl.git/commit/?id=4d69f472fe31a9006436a05282a8376c7a712d41
> >>
> >> commit 4d69f472fe31a9006436a05282a8376c7a712d41
> >> Author: Cedric Bail <ced...@osg.samsung.com>
> >> 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
> 


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

Reply via email to