On Wed, Jan 25, 2017 at 11:57 AM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> On 01/23, Junio C Hamano wrote:
>> Brandon Williams <bmwill@xxxxxxxxxx> 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.