Re: [PATCH 2/5] thread-utils: add a threaded task queue

2015-08-28 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 +void add_task(struct task_queue *tq,
 +  int (*fct)(struct task_queue *tq, void *task),

 Might make sense to typedef this... Maybe task_t?

Let's not introduce user defined type that ends with _t that is seen
globally.

 +  void *task)
 +{
 +if (tq-early_return)
 +return;

 Ah, so early_return actually means interrupted or canceled?

 I guess I will have to set aside some time to wrap my head around the
 way tasks are handled here, in particular how the two `early_return`
 variables (`dispatcher()`'s local variable and the field in the
 task_queue`) interact.

We had a very similar conversation in $gmane/276324 as the
early-return and get_task interaction was not quite intuitive.

I thought Stefan said something about this part of the logic being
unreadable and needs rework.  Perhaps that will come in the next
reroll, or something?

I tend to agree with you that interrupted or cancelled would be a
good name for this thing; at least it would help understanding what
is going on than early-return.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] thread-utils: add a threaded task queue

2015-08-27 Thread Johannes Schindelin 
Hi Stefan,

On 2015-08-27 02:52, Stefan Beller wrote:

 diff --git a/run-command.c b/run-command.c
 index 28e1d55..cb15cd9 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -668,6 +668,22 @@ int git_atexit(void (*handler)(void))
  
  #endif
  
 +void setup_main_thread(void)
 [...]

 diff --git a/thread-utils.c b/thread-utils.c
 index a2135e0..b45ab92 100644
 --- a/thread-utils.c
 +++ b/thread-utils.c
 [...]
 +/* FIXME: deduplicate this code with run-command.c */
 +static void setup_main_thread(void)

Do you remember off-hand why the code could not be moved to thread-utils.c 
wholesale? Just curious.

 +#else /* NO_PTHREADS */
 +
 +struct task_queue {
 + int early_return;
 +};
 +
 +struct task_queue *create_task_queue(unsigned max_threads)
 +{
 + struct task_queue *tq = xmalloc(sizeof(*tq));
 +
 + tq-early_return = 0;
 +}
 +
 +void add_task(struct task_queue *tq,
 +   int (*fct)(struct task_queue *tq, void *task),

Might make sense to typedef this... Maybe task_t?

 +   void *task)
 +{
 + if (tq-early_return)
 + return;

Ah, so early_return actually means interrupted or canceled?

I guess I will have to set aside some time to wrap my head around the way tasks 
are handled here, in particular how the two `early_return` variables 
(`dispatcher()`'s local variable and the field in the `task_queue`) interact.

Thanks!
Dscho
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] thread-utils: add a threaded task queue

2015-08-27 Thread Stefan Beller 
On Thu, Aug 27, 2015 at 5:59 AM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
 Hi Stefan,

 On 2015-08-27 02:52, Stefan Beller wrote:

 diff --git a/run-command.c b/run-command.c
 index 28e1d55..cb15cd9 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -668,6 +668,22 @@ int git_atexit(void (*handler)(void))

  #endif

 +void setup_main_thread(void)
 [...]

 diff --git a/thread-utils.c b/thread-utils.c
 index a2135e0..b45ab92 100644
 --- a/thread-utils.c
 +++ b/thread-utils.c
 [...]
 +/* FIXME: deduplicate this code with run-command.c */
 +static void setup_main_thread(void)

 Do you remember off-hand why the code could not be moved to thread-utils.c 
 wholesale? Just curious.

The code in run-command has a few things regarding the struct async
handling in there,
which we don't  need/want. I just realized there is some duplicate
code, but I couldn't cut
it clearly out.


 +#else /* NO_PTHREADS */
 +
 +struct task_queue {
 + int early_return;
 +};
 +
 +struct task_queue *create_task_queue(unsigned max_threads)
 +{
 + struct task_queue *tq = xmalloc(sizeof(*tq));
 +
 + tq-early_return = 0;
 +}
 +
 +void add_task(struct task_queue *tq,
 +   int (*fct)(struct task_queue *tq, void *task),

 Might make sense to typedef this... Maybe task_t?

 +   void *task)
 +{
 + if (tq-early_return)
 + return;

 Ah, so early_return actually means interrupted or canceled?

The early_return is meant to return early in case of an error in some thread.
In the threaded version, the `dispatcher` is executed in each thread.
It gets its
new tasks via `next_task`, which takes the early_return value from the thread,
ORs it into the early_return of the task queue (which all threads have
access to).
So by the ORing into the task queues early_return the signal to abort
early is propagated to a place all threads have access to. And in case
that value is
set, the `next_task` will return NULL as an indication to cleanup and
pthread_exit.


 I guess I will have to set aside some time to wrap my head around the way 
 tasks are handled here, in particular how the two `early_return` variables 
 (`dispatcher()`'s local variable and the field in the `task_queue`) interact.

 Thanks!
 Dscho
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html