Re: [E-devel] [RFC] Evas_Rectangles for evas_object_render_pre_effect_updates

[Cedric B. <ced...@fr...>]

On Fri, Jun 6, 2008 at 4:26 PM, Gustavo Sverzut Barbieri
<bar...@pr...> wrote:
> On Fri, Jun 6, 2008 at 6:05 AM, Cedric BAIL <ced...@fr...> wrote:
>> With the last serie of patch I committed inside evas, evas_render
>> should now be working again correctly. This fixed version only give
>> around 7% of speedup to expedite test and some are a little bit slower
>> than the previous.
>>
>> So after diging a little bit more the slower tests case of expedite, I
>> think the attached patch will make all expedite test as fast as
>> without the cache inside evas_render and give a global 10% speedup to
>> expedite without any cache.
>>
>> The idea is to remove as much allocation and list manipulation as
>> possible by implementing a special array of Evas_Rectangle. This patch
>> work for me on all my application, E17 and edje old test application.
>>
>> So as always have fun reviewing this patch !
>
>
> Looks good in general, just think we should use inverted names like
> the rest of evas:
>
>   evas_add_rect -> evas_rectangles_add()

Right.

> -       obj->clip.changes = updates;
> +       for (i = 0; i < rects->count; ++i)
> +         {
> +            r = malloc(sizeof(Evas_Rectangle));
> +            if (!r) goto end;
> +
> +            *r = rects->array[i];
> +            obj->clip.changes = evas_list_append(obj->clip.changes, r);
> +         }
>
> no way to avoid this? Why not change obj->clip.changes to be a pointer
> to rects? This way we save this extra walk and lots of small malloc()
> and overhead of lists (memory fragmentation, extra pointer for
> "->data" and no evas_memorypool...)... also eliminating the need to
> free the rects->array right below:

In fact before changing this I did run expedite with a printf to see
if this code was called... and I never saw the printf. So I didn't
took the time to improve this :-)

> + end:
> +   free(rects->array);
> +   rects->array = NULL;
> +   rects->count = 0;
> +   rects->total = 0;
>
> Just remember that rects might be a pointer in the stack, if so you
> need to malloc() + memcpy() it.

Yes, that why I don't free(rects) and just the array.

> +   if (obj->smart.smart) return ;
> +   if (is_v == was_v) return ;
>
> why do you like this space between "return" and ";"  ?   :-P

:-D

> +/*     if (tmp.count > 0) */
> +/*       { */
> +/*          unsigned int i; */
> +
> +/*          for (i = 0; i < tmp.count; ++i) */
> +/*            { */
> +/*               if ((tmp.array[i].w > 0) && (tmp.array[i].h > 0)) */
> +/*                 { */
> +/*                    int intsec1, intsec2; */
> +
> +/*                    intsec1 = (RECTS_INTERSECT(tmp.array[i].x,
> tmp.array[i].y, tmp.array[i].w, tmp.array[i].h, x, y, w, h)); */
> +/*                    intsec2 = (RECTS_INTERSECT(tmp.array[i].x,
> tmp.array[i].y, tmp.array[i].w, tmp.array[i].h, xx, yy, ww, hh)); */
> +/*                    if (intsec1 ^ intsec2) */
> +/*                      { */
> +/*                         evas_add_rect(rects, tmp.array[i].x, tmp.array[i].y,
> tmp.array[i].w, tmp.array[i].h); */
> +/*                      } */
> +/*                 } */
> +/*            } */
> +/*          free(tmp.array); */
> +/*       } */
>
> what's this? remove or uncomment?

Because I didn't really understood why it needed two list in the
previous version. So I keep it around just in case.

> +   if ((rects->count + 1) > rects->total)
> +     {
> +       Evas_Rectangle *_add_rect;
> +       unsigned int _tmp_total;
> +
> +       _tmp_total = rects->total + 32;
> +       _add_rect = realloc(rects->array, sizeof(Evas_Rectangle) * _tmp_total);
> +       if (!_add_rect) return ;
> +
> +       rects->total = _tmp_total;
> +       rects->array = _add_rect;
>
> and
>
> +   if (rects->max < (rects->active + 1))
> +     {
> +       rects->max += 32;
> +       rects->rects = realloc(rects->rects, sizeof(Cutout_Rect) * rects->max);
> +     }
>
> "so close, yet so far...". Make them "almost the same", the second
> version doesn't handle any error. also change the comparison to be
> uniform.

You already saw the next discussion I would like to start ! :-)

Cutout_Rect are really the same as Evas_Rectangles in their definition
and they are allocated/reallocated a lot between two call to
evas_render. The same for Evas_Rectangles. In fact, they are  the main
source of call to malloc/realloc and in some expedite test, it's
consuming around 3 to 5% of the application time.

Of course the first thing we should do is to merge both
Evas_Rectangles and Cutout_Rects.

But the most important improvement would be to keep all this
Cutout_Rects and Evas_Rectangles arrays around between to call to
evas_render and we could flush them only when needed. I just don't
know where to keep them.

Oh, and if someone know how to improve
evas_common_draw_context_cutout_split, it's consume around 20% of
expedite time in many tests.
-- 
Cedric BAIL