Re: regression in multi-threaded git-pack-index

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 19, 2013, 12:35 UTC
Message-ID: <CACsJy8DgQZFewPjLSXSkdHHWqhQDqExoVq-pBGpKr1G8w06uvQ@mail.gmail.com>
In-Reply-To: <87fvzrajmr.fsf@pctrast.inf.ethz.ch>

On Tue, Mar 19, 2013 at 3:17 PM, Thomas Rast <trast@student.ethz.ch> wrote:

Show 13 quoted lines

>> but the line in question is:
>>
>>   if (deepest_delta < delta_obj->delta_depth)
>>
>...
>
> Duy, what was the reasoning why resolve_delta() does not need to hold
> locks when it looks when it looks at deepest_delta?  My coffee levels
> aren't up to this task yet.  It certainly seems extremely dubious to me,
> as the code uses the global deepest_delta in threaded sections.  You can
> probably argue that the load/store is atomic on most(?) platforms, but
> you get no guarantees that deepest_delta at any time in fact holds the
> maximum value of delta_obj->delta_depth.

Now that I have had dinner (and energy restored), the explanation might be because I missed it. resolve_delta() deals with data in delta_obj and does not share global state, except this one and nr_resolved_deltas. The latter is protected. I guess we could protect this one with a mutex. But only do so when "--verify" is specified

if (stat) {
   lock();
   if (deepest_delta < delta_obj->delta_depth)
      deepest_delta = delta_obj->delta_depth;
   unlock();
}

so that we don't need to hold/release lock when index-pack is run.

-- 
Duy