[cairo] [PATCH 1/3] cairo_rwlock: introduce rwlock.

[Uli Schlachter]

Hi,

On 25.05.2012 11:13, Zhigang Gong wrote:
> Here is a scenario which make us to introduce rwlock.
> 1. One thread(A) is rendering to a surface which has a snapshot attached.
> 2. Some other threads(B...) are replaying recording surfaces which has the
> snapshot of the surface which is being rendered in thread 1.

Could you also write a test case for the test suite? That would make sure that
this actually fixes what it wants to fix (and makes it easier to see what that is).

[...]
> Currently, we only implement pthread_rw_lock if the system supports
> pthread. Otherwise, we degrade the rw_lock to a normal lock.

Why is "a normal lock" not a cairo_mutex_t? It looks like this patch just copied
all the non-pthread code from cairo mutex.

[...]
> diff --git a/src/cairo-rwlock-impl-private.h b/src/cairo-rwlock-impl-private.h
> new file mode 100644
> index 0000000..67a8fc9
> --- /dev/null
> +++ b/src/cairo-rwlock-impl-private.h
> @@ -0,0 +1,135 @@
[...]
> +#ifndef CAIRO_RWLOCK_IMPL_PRIVATE_H
> +#define CAIRO_RWLOCK_IMPL_PRIVATE_H
> +#include "cairo.h"
> +
> +#if HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +/* A fully qualified no-operation statement */
> +#define CAIRO_RWLOCK_IMPL_NOOP	do {/*no-op*/} while (0)
> +/* And one that evaluates its argument once */
> +#define CAIRO_RWLOCK_IMPL_NOOP1(expr)        do { (void)(expr); } while (0)
> +/* Note: 'if (expr) {}' is an alternative to '(void)(expr);' that will 'use' the
> + * result of __attribute__((warn_used_result)) functions. */
> +
> +#if CAIRO_NO_RWLOCK
> +
> +/* No rwlocks */
> +
> +  typedef int cairo_rwlock_impl_t;
> +
> +# define CAIRO_RWLOCK_IMPL_NO 1
> +# define CAIRO_RWLOCK_IMPL_LOCK(rwlock) CAIRO_RWLOCK_IMPL_NOOP1(rwlock)
> +# define CAIRO_RWLOCK_IMPL_UNLOCK(rwlock) CAIRO_RWLOCK_IMPL_NOOP1(rwlock)
> +# define CAIRO_RWLOCK_IMPL_NIL_INITIALIZER 0
> +
> +#elif defined(_WIN32) /******************************************************/
> +
> +#define WIN32_LEAN_AND_MEAN
> +/* We require Windows 2000 features such as ETO_PDY */
> +#if !defined(WINVER) || (WINVER < 0x0500)
> +# define WINVER 0x0500
> +#endif
> +#if !defined(_WIN32_WINNT) || (_WIN32_WINNT < 0x0500)
> +# define _WIN32_WINNT 0x0500
> +#endif
> +
> +# include <windows.h>
> +
> +  typedef CRITICAL_SECTION cairo_rwlock_impl_t;
> +
> +# define CAIRO_RWLOCK_IMPL_WIN32 1
> +# define CAIRO_RWLOCK_IMPL_RDLOCK(rwlock) EnterCriticalSection (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_WRLOCK(rwlock) EnterCriticalSection (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_UNLOCK(rwlock) LeaveCriticalSection (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_INIT(rwlock) InitializeCriticalSection (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_FINI(rwlock) DeleteCriticalSection (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_NIL_INITIALIZER { NULL, 0, 0, NULL, NULL, 0 }
> +
> +#elif defined __OS2__ /******************************************************/
> +
> +# define INCL_BASE
> +# define INCL_PM
> +# include <os2.h>
> +
> +  typedef HMTX cairo_rwlock_impl_t;
> +
> +# define CAIRO_RWLOCK_IMPL_OS2 1
> +# define CAIRO_RWLOCK_IMPL_RDLOCK(rwlock) DosRequestMutexSem(rwlock, SEM_INDEFINITE_WAIT)
> +# define CAIRO_RWLOCK_IMPL_WRLOCK(rwlock) DosRequestMutexSem(rwlock, SEM_INDEFINITE_WAIT)
> +# define CAIRO_RWLOCK_IMPL_UNLOCK(rwlock) DosReleaseMutexSem(rwlock)
> +# define CAIRO_RWLOCK_IMPL_INIT(rwlock) DosCreateMutexSem (NULL, &(rwlock), 0L, FALSE)
> +# define CAIRO_RWLOCK_IMPL_FINI(rwlock) DosCloseMutexSem (rwlock)
> +# define CAIRO_RWLOCK_IMPL_NIL_INITIALIZER 0
> +
> +#elif CAIRO_HAS_BEOS_SURFACE /***********************************************/
> +
> +  typedef BLocker* cairo_rwlock_impl_t;
> +
> +# define CAIRO_RWLOCK_IMPL_BEOS 1
> +# define CAIRO_RWLOCK_IMPL_RDLOCK(rwlock) (rwlock)->Lock()
> +# define CAIRO_RWLOCK_IMPL_WRLOCK(rwlock) (rwlock)->Lock()
> +# define CAIRO_RWLOCK_IMPL_UNLOCK(rwlock) (rwlock)->Unlock()
> +# define CAIRO_RWLOCK_IMPL_INIT(rwlock) (rwlock) = new BLocker()
> +# define CAIRO_RWLOCK_IMPL_FINI(rwlock) delete (rwlock)
> +# define CAIRO_RWLOCK_IMPL_NIL_INITIALIZER NULL
> +
> +#elif CAIRO_HAS_PTHREAD /* and finally if there are no native rwlocks ********/
> +
> +# include <pthread.h>
> +  typedef pthread_rwlock_t cairo_rwlock_impl_t;
> +
> +# define CAIRO_RWLOCK_IMPL_PTHREAD 1
> +# define CAIRO_RWLOCK_IMPL_INIT(rwlock) pthread_rwlock_init (&(rwlock), NULL)
> +# define CAIRO_RWLOCK_IMPL_RDLOCK(rwlock) pthread_rwlock_rdlock (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_WRLOCK(rwlock) pthread_rwlock_wrlock (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_UNLOCK(rwlock) pthread_rwlock_unlock (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_FINI(rwlock) pthread_rwlock_destroy (&(rwlock))
> +# define CAIRO_RWLOCK_IMPL_NIL_INITIALIZER PTHREAD_RWLOCK_INITIALIZER
> +
> +#else /**********************************************************************/
> +
> +# error "XXX: No rwlock implementation found.  Cairo will not work with multiple threads.  Define CAIRO_NO_RWLOCK to 1 to acknowledge and accept this limitation and compile cairo without thread-safety support."
> +
> +#endif

All of the above should be replaceable with:

#if CAIRO_HAS_PTHREAD
[the pthread stuff as-is above]
#else
typedef cairo_mutex_t cairo_rwlock_impl_t
# define CAIRO_RWLOCK_IMPL_INIT(rwlock) CAIRO_MUTEX_INIT(rwlock)
# define CAIRO_RWLOCK_IMPL_RDLOCK(rwlock) CAIRO_MUTEX_LOCK(rwlock)
[etc]
#endif

Do I miss a point which makes this not possible?

[...]
> diff --git a/src/cairo-rwlock.c b/src/cairo-rwlock.c
> new file mode 100644
> index 0000000..492716e
> --- /dev/null
> +++ b/src/cairo-rwlock.c
> @@ -0,0 +1,56 @@
[...]
> + * The Original Code is the cairo graphics library.
> + *
> + * Contributor(s):
> + *	Mathias Hasselmann <mathias.hasselmann at gmx.de>

Who is Mathias? Is this copy&paste from cairo-mutex.c?

> +#include "cairoint.h"
> +
> +#include "cairo-rwlock-private.h"
> +
> +static int _cairo_rwlock_initialized = 0;
> +
> +void _cairo_rwlock_initialize (void)
> +{
> +    if (_cairo_rwlock_initialized)
> +        return;
> +
> +    _cairo_rwlock_initialized = TRUE;
> +
> +}
> +
> +void _cairo_rwlock_finalize (void)
> +{
> +    if (!_cairo_rwlock_initialized)
> +        return;
> +
> +    _cairo_rwlock_initialized = FALSE;
> +
> +}

What the heck? Just remove all of this (including the stuff for this in the
headers) and done? This doesn't do anything at all and it's not even called from
anywhere, is it?

Uli
-- 
"Do you know that books smell like nutmeg or some spice from a foreign land?"
                                                  -- Faber in Fahrenheit 451