On 01/23, Junio C Hamano wrote: > Brandon Williams <bmw...@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
