Re: [PATCH] index-pack: protect deepest_delta in multithread code

[Duy Nguyen]

On Tue, Mar 19, 2013 at 8:50 PM, Thomas Rast  wrote:
> -- >8 --
> Subject: [PATCH] index-pack: guard nr_resolved_deltas reads by lock
>
> The threaded parts of index-pack increment the number of resolved
> deltas in nr_resolved_deltas guarded by counter_mutex.  However, the
> per-thread outer loop accessed nr_resolved_deltas without any locks.
>
> This is not wrong as such, since it doesn't matter all that much
> whether we get an outdated value.  However, unless someone proves that
> this one lock makes all the performance difference, it would be much
> cleaner to guard _all_ accesses to the variable with the lock.
>
> Signed-off-by: Thomas Rast 
> ---
>  builtin/index-pack.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index b3fee45..6be99e2 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -968,7 +968,9 @@ static void *threaded_second_pass(void *data)
> for (;;) {
> int i;
> work_lock();
> +   counter_lock();
> display_progress(progress, nr_resolved_deltas);
> +   counter_unlock();
> while (nr_dispatched < nr_objects &&
>is_delta_type(objects[nr_dispatched].type))
> nr_dispatched++;

I'm pretty sure futex will make this cheap. The only thing I don't
like here is the double locking (work_lock then counter_lock) is an
invitation for potential deadlocks (not now, but who now what can
change later). I think you could move work_lock(); down after
counter_unlock() so we hold one lock at a time.
-- 
Duy
--
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] index-pack: protect deepest_delta in multithread code

[Thomas Rast]

Nguyễn Thái Ngọc Duy  writes:

> deepest_delta is a global variable but is updated without protection
> in resolve_delta(), a multithreaded function. Add a new mutex for it,
> but only protect and update when it's actually used (i.e. show_stat is
> non-zero).
>
> Another variable that will not be updated is delta_depth in "struct
> object_entry" as it's only useful when show_stat is 1. Putting it in
> "if (show_stat)" makes it clearer.
>
> The local variable "stat" is renamed to "show_stat" after moving to
> global scope because the name "stat" conflicts with stat(2) syscall.

Looks good to me, too.  However, I think it would still be less magical
if we had the below too.  It also silences helgrind.

-- >8 --
Subject: [PATCH] index-pack: guard nr_resolved_deltas reads by lock

The threaded parts of index-pack increment the number of resolved
deltas in nr_resolved_deltas guarded by counter_mutex.  However, the
per-thread outer loop accessed nr_resolved_deltas without any locks.

This is not wrong as such, since it doesn't matter all that much
whether we get an outdated value.  However, unless someone proves that
this one lock makes all the performance difference, it would be much
cleaner to guard _all_ accesses to the variable with the lock.

Signed-off-by: Thomas Rast 
---
 builtin/index-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index b3fee45..6be99e2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -968,7 +968,9 @@ static void *threaded_second_pass(void *data)
for (;;) {
int i;
work_lock();
+   counter_lock();
display_progress(progress, nr_resolved_deltas);
+   counter_unlock();
while (nr_dispatched < nr_objects &&
   is_delta_type(objects[nr_dispatched].type))
nr_dispatched++;
-- 
1.8.2.487.g725f6bb


-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
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] index-pack: protect deepest_delta in multithread code

[Jeff King]

On Tue, Mar 19, 2013 at 08:01:15PM +0700, Nguyen Thai Ngoc Duy wrote:

> deepest_delta is a global variable but is updated without protection
> in resolve_delta(), a multithreaded function. Add a new mutex for it,
> but only protect and update when it's actually used (i.e. show_stat is
> non-zero).

This makes sense to me.

> Another variable that will not be updated is delta_depth in "struct
> object_entry" as it's only useful when show_stat is 1. Putting it in
> "if (show_stat)" makes it clearer.

Having just read through this code for the first time, I agree that
having the "if (show_stat)" would have made it a lot more clear under
what conditions and for what purpose the delta_depth flag was being
used.

>  builtin/index-pack.c | 30 +++---
>  1 file changed, 23 insertions(+), 7 deletions(-)

Patch looks good to me. Thanks.

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