Re: [PATCH v3] Threaded grep

[Johannes Sixt <j.sixt@viscovery.net>]

Fredrik Kuivinen schrieb:
> The patch has been rebased on top of next and I believe that it is now
> ready for inclusion. It is time to decide if the added code complexity
> is worth the increased performance.

I also have to add a few nits to make this play better on Windows.

> +/* This lock protects all the variables above. */
> +static pthread_mutex_t grep_lock = PTHREAD_MUTEX_INITIALIZER;
> +
> +/* Signalled when a new work_item is added to todo. */
> +static pthread_cond_t cond_add = PTHREAD_COND_INITIALIZER;
> +
> +/* Signalled when the result from one work_item is written to
> +   stdout. */
> +static pthread_cond_t cond_write = PTHREAD_COND_INITIALIZER;
> +
> +/* Signalled when we are finished with everything. */
> +static pthread_cond_t cond_result = PTHREAD_COND_INITIALIZER;

Please do not use PTHREAD_MUTEX_INITIALIZER nor PTHREAD_COND_INITIALIZER;
call pthread_mutex_init and pthread_cond_init (and the corresponding
*_destroy!!) from the code.

> +static void add_work(enum work_type type, char *name, char *buf, size_t size)
> +{
> +...
> +	pthread_mutex_unlock(&grep_lock);
> +	pthread_cond_signal(&cond_add);

Please swap these two lines, so that pthread_cond_signal() is called while
the lock is held.

> +	/* Wake up all the consumer threads so they can see that there
> +	   is no more work to do. */
> +	pthread_cond_broadcast(&cond_add);
> +	pthread_mutex_unlock(&grep_lock);

Ouch! We do not have pthread_cond_broadcast() on Windows, yet. This
shouldn't be a stopper for this patch, though. Perhaps Andrzej (cc:ed) can
implement it?

-- Hannes