Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure

From: Brandon Williams <bmwill@google.com>
Date: Jan 25, 2017, 19:57 UTC
Message-ID: <20170125195745.GA83343@google.com>
In-Reply-To: <xmqqinp5p4x2.fsf@gitster.mtv.corp.google.com>

On 01/23, Junio C Hamano wrote:

Show 10 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> 
> > ... It seems like breaking the question and answer up
> > doesn't buy you much in terms of reducing allocation churn and instead
> > complicates the API with needing to keep track of two structures instead
> > of a one.
> 
> In my mind, the value of having a constant check_attr is primarily
> that it gives us a stable pointer to serve as a hashmap key,
> i.e. the identifier for each call site, in a later iteration.

We didn't really discuss this notion of having the pointer be a key into a hashmap, what sort of information are you envisioning being stored in this sort of hashmap? One issue I can see with this is that the functions which have a static attr_check struct would still not be thread safe if the initialization of the structure isn't surrounded by a mutex itself. ie

static struct attr_check *check;

if (!check)
  init(check);

would need to be:

lock()
if (!check)
  init(check);
unlock();

inorder to prevent a race to initialize the structure. Which is something that the attr system itself can't be refactored to fix (at least I can't see how at the moment).

> Of course, in order to populate the "question" array, we'd need the
> interning of attribute names to attr objects, which need to be
> protected by mutex, and you would probably not want to do that every
> time the control hits the codepath.

While true that doesn't prevent the mutex needed to create/check that the all_attr array that is used to collect attributes is the correct size/initialized properly.

> But all of the above comes from my intuition, and I'll very much
> welcome to be proven wrong with an alternative design, or better
> yet, a working code based on an alternative design ;-).

Yeah, after working through the problem the two simple solutions I can think of are either my v1 or v2 of the series, neither of which allows for the attr_check structure to be shared. If we truly want the "question" array to be const then that can be done, it would just require a bit more boilerplate and making the all_attr array to be local to the check_attrs() function itself. An API like this would look like:

static const struct attr_check *check; struct attr_result result;

if (!check)
  init_check(check);

// Result struct needs to be initialized based on the size of check init_result(&result, check);

check_attrs(path, check, &result);

// use result

attr_result_clear(&result);

It still doesn't handle an initialization race on the check structure but the check pointer would be const and could be used for some future optimization. It also will have a bit more allocation churn than either v1 or v2 of the series. If this is the route you want to take I'll get working on it, I just want to make sure we're on the same page before doing a larger refactor like this.

Thanks for the guidance on this, someday we'll get this right :)

-- 
Brandon Williams