Re: [PATCH v2 7/7] index-pack: make quantum of work smaller

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 10, 2020, 21:42 UTC
Message-ID: <20200310214206.81606-1-jonathantanmy@google.com>
In-Reply-To: <20200228000459.GE12115@google.com>

Show 32 quoted lines

> > +/*
> > + * Stack of struct base_data that have unprocessed children.
> > + * threaded_second_pass() uses this as a source of work (the other being the
> > + * objects array).
> > + */
> > +LIST_HEAD(work_head);
> > +
> > +/*
> > + * Stack of struct base_data that have children, all of whom have been
> > + * processed or are being processed, and at least one child is being processed.
> > + * These struct base_data must be kept around until the last child is
> > + * processed.
> > + */
> > +LIST_HEAD(done_head);
> > +
> > +/*
> > + * All threads share one delta base cache.
> > + */
> > +size_t base_cache_used;
> > +size_t base_cache_limit;
> > +
> >  struct thread_local {
> >  	pthread_t thread;
> > -	size_t base_cache_used;
> >  	int pack_fd;
> >  };
> 
> Seeing several new shared variables made me paranoid about whether or
> not these were thread-safe. There are already five mutexes in this file,
> but there are no clear descriptions (apart from hints in the lock names)
> about which locks protect which variables. It would be nice to make this
> a bit more explicit.

I think that this patch set should confine itself to the work mutex and what it controls (since that's what this patch set is affecting), but admittedly, that is already more than a handful.

The work mutex controls work_head and done_head, and all the struct base_data contained therein, except for data. However, data is controlled by the semaphore retain_data (and the semaphore is itself controlled by the work mutex, as described in the previous sentence): when retain_data is positive, data can be read from but not written to. The work mutex also controls base_cache_used but not base_cache_limit. The work mutex does not control any struct base_data outside of work_head and done_head, so it is possible to generate a struct base_data outside the mutex, take the mutex, and add it to the list (reducing time spent in the mutex).

This scheme seems rather complicated to me, but I couldn't make it simpler - I was hoping for advice on this. If this is the best we can do, I can try to document it in code comments.

Show 5 quoted lines

> I wish we had something similar to Abseil's "GUARDED_BY" annotations [1]
> so that we could ensure thread safety with static analysis, but that's
> obviously out of scope for this series.
> 
> [1]: https://abseil.io/docs/cpp/guides/synchronization#thread-annotations

I think what we need is more complicated than what these annotations provide, but anyway, Abseil is C++-only, I believe.

Show 29 quoted lines

> > @@ -1065,33 +1011,128 @@ static int compare_ref_delta_entry(const void *a, const void *b)
> [snip]
> >  		work_unlock();
> >  
> > -		resolve_base(&objects[i]);
> > +		if (parent) {
> > +			child = resolve_delta(child_obj, parent);
> > +			if (!child->children_remaining)
> > +				FREE_AND_NULL(child->data);
> > +		} else {
> > +			child = make_base(child_obj, NULL);
> > +			if (child->children_remaining) {
> > +				/*
> > +				 * Since this child has its own delta children,
> > +				 * we will need this data in the future.
> > +				 * Inflate now so that future iterations will
> > +				 * have access to this object's data while
> > +				 * outside the work mutex.
> > +				 */
> > +				child->data = get_data_from_pack(child_obj);
> > +				child->size = child_obj->size;
> > +			}
> > +		}
> > +
> > +		work_lock();
> 
> This part made me uneasy at first, because it looks like we might be
> doing work on shared data outside of the work lock. But just prior to
> unlocking, we parent->retain_data, so we know it won't be freed.

That's correct that it made you uneasy (I think the usual solution is to make data refcounted, but we don't have a refcounting implementation in Git), and that's correct that my solution was that parent->retain_data prevents data from being concurrently written to (in this case, freed). I didn't know how better to implement this.

> And the
> only part we modify is child, which I don't believe is shared with any
> other threads.

Yes - in the part you quoted, child is a whole new object (created from resolve_delta() or make_base()). It is only added to the list within the mutex (the work_lock()).