[Pixman] [RFC] Performance statistics analyzer

[Andrea Canciani]

On Wed, Sep 7, 2011 at 6:46 AM, Maarten Bosmans <mkbosmans at gmail.com> wrote:
> 2011/9/6 Taekyun Kim <podain77 at gmail.com>:
>> Hi,
>>
>> Currently pixman is used as a lowlevel backend for various applications.
>> For doing analysis on performance, it might be desirable to provide some
>> performance statistics information to developers.
>>
>> The basic idea is based on siarhei's slow-path-reporter. I extended it to
>> accumulate information of every composite paths. Final result will be
>> shown at the end of the application when destructor is called. Each paths
>> are sorted by the amount of time they spent.
>
> Indeed, this is very useful.
>
>> The main purpose of this utility is to profile cairo traces. We can easily
>> figure out hot spots and real world usage patterns.
>>
>> You can see it from here.
>>
>>     http://cgit.freedesktop.org/~podain/pixman/?h=perf_stat
>>
>> You can give --enable-perf-stat option to configure script to enable the
>> performance statistics analyzer. (I'm not sure that this is done right,
>> because I'm not familiar with autoconf, but it works anyway)
>>
>> Is it right or good to include this feature within upstream? I think other
>> developers can easily profile their applications and report the statistics
>> when they suffer from performance.
>>
>> I was thinking of some performance log channel, individual composite
>> event is printed though that log channel and statistics analysis utilities
>> to retrieve meaningful information from the log.
>>
>> All suggestions and comments are welcome.
>
> I took this RFC to also be a request for patches.
> Attached is a patch that strips out the pthread mutex usage and
> implements some macros in pixman-compiler.h, not unlike the TLS
> macros. I also added a win32 implementation and tested it using mingw.

Is there any reason to use win32 Mutexes instead of CriticalRegions?
AFAIK Mutexes are for inter-process synchronization, while each
CriticalRegion can only be used by a single process to synchronize
multiple threads (and has a better performance, because most of the
times it does not require calling into the kernel).

>
> Some further improvements could be: check whether the win32
> implementation works with MSVC and use it there too. And also a way of
> opting out if you're on a non-win32, non-pthread platform and don't
> want multithreading.

It does not work with MSVC because it obvously does not provide the
__MINGW32__ definition. Replacing it with _WIN32 fixes that build
error but still dies in pixman-fast-path.c because "window.h" defines
IN and OUT. I have a patch for that in my simpleops branch, I'll try
cherry-picking it.

About the previous part of the patchset:
Would it be possible to avoid the #ifdef PIXMAN_PERF_STAT in
pixman.c and just make perfstat_is_enabled() an inline function
which always returns false when it is not defined?
I believe that the code would look cleaner.

(We might actually need to ifdef the code out in order to be able
to compile when no mutexes are available, but the branch currently
does not try to detect it AFAICT)

Andrea