Thread (3 messages) flat view 3 messages, 2 authors, 2016-06-15

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

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:56:26

Possibly related (same subject, not in this thread)

On Tue, Mar 19, 2013 at 8:50 PM, Thomas Rast [off-list ref] wrote:
quoted hunk ↗ jump to hunk
-- >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 <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help