[PATCH] abg-workers: Rework the worker queue to improve concurrent behaviour

[Dodji Seketeli]

Hello Matthias,

Matthias Maennich <maennich@google.com> a écrit:

> This patch refactors the abigail::workers::queue and
> abigail::workers::worker implementations to avoid holding locking
> primitives longer than necessary.
>
> In particular, the queue_cond_mutex was held during the entiry worker
> runtime, effectively serializing the workers. Hence, use a mutex+cond
> pair for each, the input and output queue and only synchronize around
> the interaction with their corresponding queues. The
> tasks_todo_(mutex|cond) are meant to synchronize scheduling and
> distribution of work among workers, while tasks_done_(mutex|cond) are
> used for synchronizing threads when putting back the tasks to the output
> queue and to hold back threads waiting for the queue and workers to
> drain.
>
> Along that way, I did some cleanup that was now possible.
>  - Move entire implementation of abigail::workers::task into header.
>  - Make default_notify a static member.
>  - Replace the multiple constructors with one with default arguments.
>
> 	* include/abg-workers.h (workers::task): move entire
> 	implementation to header and drop superfluous forward declaration.
> 	* src/abg-workers.cc (workers::task):: Likewise.
> 	(workers::queue::priv): Drop queue_cond_mutex, rename queue_cond
> 	to tasks_todo_cond, add task_done_cond, make default_notify
> 	static.
> 	(workers::queue::priv::priv): Add default arguments to fully
> 	qualified constructor, drop the remaining ones.
> 	(workers::queue:prive::more_tasks_to_execute): Drop method.
> 	(workers::queue:prive::schedule_task): Do not synchronize access
> 	to the queue condition variable, but only on the mutex.
> 	(do_bring_workers_down): Likewise. Also await tasks_done to be
> 	empty.
> 	(workers::queue:prive::worker::wait_to_execute_a_task): Await
> 	tasks on the tasks_todo with tasks_todo_(cond|mutex) and signal
> 	task completion to tasks_done_cond.

Whoah, thanks for looking into this!

The patch looks good to me, and I have applied to master.

I have changed a few stylistic nits, namely:

[...]


>    // The todo task queue itself.
> -  std::queue<task_sptr>	tasks_todo;
> +  std::queue<task_sptr>		tasks_todo;

I removed this useless white space that was added here.

>    // The done task queue itself.
>    std::vector<task_sptr>	tasks_done;
> +

I removed this added vertical space here.

[...]

>  
> -    // Acquire the mutex that protects the queue condition variable
> -    // (queue_cond) and wake up all the workers that are sleeping on
> -    // the condition.
> -    pthread_mutex_lock(&queue_cond_mutex);
> +    // Wait for the todo list to be empty to make sure all tasks got picked up
> +    pthread_mutex_lock(&tasks_todo_mutex);
> +    while (!tasks_todo.empty()) {
> +	pthread_cond_wait(&tasks_done_cond, &tasks_todo_mutex);
> +    }

In general, we put the opening brace alone on its own line.  But in this
case, because there is only one statement in the body of the while-loop,
we don't put any opening/close brace at all.  So I removed them
altogether.

Thanks a lot!

Cheers,

-- 
		Dodji