[Pixman] [PATCH 0/8] Performance statistics analyzer

[Søren Sandmann]

Hi,

> Pixman is used as a bottom-layer backend for S/W rendering for various
> applications and majority of the time is spent by pixman for rendering
> intensive work. So providing performance information can help people
> figure out hot spots and we can do optimizations based on the result.
>
> This profiling tool is an extended version of siarhei's slow path
> reporter and gathers performance data for each compositing paths and
> report the result at the end of the program. You can include this tool
> by giving --enable-perf-stat=yes option to autoconf and turn on by
> setting PIXMAN_ENABLE_PERFSTAT environmental variable at runtime.

This looks really good in general. Being able to track how much time is
spent doing what, is going to be extremely useful.

The one highlevel comment I have is about fast_composite_solid_fill(),
where you change from calling pixman_fill() to directly calling the new
pixman_fill_fast_path(). This is problematic because pixman_fill() could
call the SSE2 (or NEON) version of fill, whereas pixman_fill_fast_path()
is always C only.

I understand that you did it this way to avoid double-reporting when a
composite operation degenerates to a solid fill, but we can't lose the
ability for fast path solid fills to use SSE2.

Instead, I'd suggest to have

         perfstat_begin_*()
         perfstat_end()

calls that would 

      (a) do the time tracking themselves, so perfstat_get_time() would
          be unnecessary

      (b) allow nesting, but disregard the inner calls. Ie., if they are
          called like this:

              perf_stat_begin_composite(...)
              perf_stat_begin_fill(...)
              perf_stat_end()
              perf_stat_end()  

          the innermost pair would be discarded.

          In order to keep track of the data, thread local variables
          could be used. Something like

                PIXMAN_DEFINE_THREAD_LOCAL (perf_stat_info_t, info);

          and then in perf_stat_begin()

                if (info->in_progress_*)
                   return;

                info->in_progress = TRUE;
          
                info->time = get_time();

          Or maybe it's more useful to have perf_stat_end_*() and just
          one perf_stat_begin().

I have some more code-specific comments too, that I'll send in
follow-ups to the individual patches.

But in general, these patches do look really good.


Thanks,
Soren