Re: [PATCH v3] Threaded grep

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Jan 18, 2010, 11:11 UTC
Message-ID: <4B5441D7.40503@viscovery.net>
In-Reply-To: <20100118103334.GA17361@fredrik-laptop>

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.

Show 12 quoted lines

> +/* 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.

Show 5 quoted lines

> +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