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

[Stefan Beller <sbeller@google.com>]

On Wed, Jan 25, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
> 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).

By passing the check pointer into the attr system (using a double pointer)

    extern void git_attr_check_initl( \
            struct git_attr_check out**, \
            const char *, ...)
{
    // get the global lock, as construction of new check structs
    // is not expected to produce contention

    // parse the list of things & construct the thing

    *out = /* I made a thing */
    // unlock globally
}

>
>> 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);

Behind the scenes we may have a pool that caches result allocations,
such that we avoid memory allocation churn in here


>
> check_attrs(path, check, &result);
>
> // use result
>
> attr_result_clear(&result);

Instead of clearing here, we'd give it back to the pool, which then can keep
parts of the result intact.