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

[Brandon Williams <bmwill@google.com>]

On 01/23, Junio C Hamano wrote:
> 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