Re: [PATCH 28/28] attr: convert to new threadsafe API

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 11, 2016 at 10:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This revamps the API of the attr subsystem to be thread safe.
>> Before we had the question and its results in one struct type.
>> The typical usage of the API was
>>
>>     static struct git_attr_check;
>
> I think you meant "*check" at the end, perhaps?
>
>>
>>     if (!check)
>>         check = git_attr_check_initl("text", NULL);
>>
>>     git_check_attr(path, check);
>>     act_on(check->value[0]);
>>
>> * While the check for attributes to be questioned only need to
>>   be initalized once as that part will be read only after its
>>   initialisation, the answer may be different for each path.
>>   Because of that we need to decouple the check and the answer,
>>   such that each thread can obtain an answer for the path it
>>   is currently processing.
>
> Yes, it is good to separate questions and answers.  I think answers
> should be owned by the caller, though.  I do not think of a good
> reason why you want to make it impossible to do something like this:
>
>         struct git_attr_check_result *result_1 = ...allocate...;
>         struct git_attr_check_result *result_2 = ...allocate...;
>
>         loop {
>                 struct strbuf path = next_path();
>                 git_check_attr(result_1, path.buf, check_1);
>                 if (strbuf_strip_suffix(&path, ".c")) {
>                         strbuf_addstr(&path, ".h");
>                         git_check_attr(result_2, path.buf, check_2);
>                         do something using result_1[] and result_2[];
>                 } else {
>                         do something using result_1[];
>                 }
>         }
>
> Do we already have a design of the "result" thing that is concrete
> enough to require us to declare that the result is owned by the
> implementation and asking another question has to destroy the answer
> to the previous question?  Otherwise, I'd rather not to see us make
> the API unnecessarily hard to use.  While I do want us to avoid
> overengineering for excessive flexibility, I somehow feel "you
> cannot control the lifetime of the result, it is owned by the
> subsystem" falls the other side of the line.

True, we had that issue for other APIs (IIRC path related things,
with 4 static buffers that round robin). I did not like that design decision,
but I felt it was okay, as the above did not occur to me.

In the case above, we could just copy the result_1->values and
then re-use the result_1, but I agree that this may be somewhat error prone
if you're not familiar with the decisions in this series.

So in case of the caller owning the result, we could pull the static
trick for now
and only use a different approach when we use it in actual threaded code, i.e.
the code in convert.c could become:

    static struct git_attr_check *check;
    static struct git_attr_result *result;

    if (!check) {
        check = git_attr_check_initl("crlf", "ident",
            "filter", "eol", "text", NULL);
        result = git_attr_result_alloc(5);
        user_convert_tail = &user_convert;
        git_config(read_convert_config, NULL);
    }

    if (!git_check_attr(path, check, result)) {
        ...

>> +  empty `struct git_attr_check` as alloced by git_attr_check_alloc()
>
> "allocated", not "alloced".

ok.

>
>> +  can be prepared by calling `git_attr_check_alloc()` function and
>> +  then attributes you want to ask about can be added to it with
>> +  `git_attr_check_append()` function.
>> +  git_attr_check_initl is thread safe, i.e. you can call it
>
> Spell it `git_attr_check_initl()` for consistency.

ok.

>
>> +  from different threads at the same time; internally however only one
>> +  call at a time is processed. If the calls from different threads have
>> +  the same arguments, the returned `git_attr_check` may be the same.
>
> I find this description a bit confusing.  At least the way I
> envisioned was that when this piece of code is run by multiple
> people at the same time,
>
>         static struct git_attr_check *check = NULL;
>         git_attr_check_initl(&check, ...);
>
> we won't waste the "check" by allocated by the first one by
> overwriting it with another one allocated by the second one.  So
> "the same arguments" does not come into the picture.  A single
> variable is either
>
>  * already allocated and initialised by the an earlier call to
>    initl() by somebody else, or
>
>  * originally NULL when you call initl(), and the implementation
>    makes sure that other people wait while you allocate, initialise
>    and assign it to the variable, or
>
>  * originally NULL when you call initl(), but the implementation
>    notices that somebody else is doing the second bullet point
>    above, and you wait until that somebody else finishes and then
>    you return without doing anything (because by that time, "check"
>    is filled by that other party doing the second bullet point
>    above).
>
> There is no need to check for "the same arguments".
>

I see. So we assume that there are no different arguments at the same time,
i.e. all threads run the same code when it comes to attrs.

Brandon wrote:
> On 10/10, Stefan Beller wrote:
>>   be initalized once as that part will be read only after its
>>       initialized
>>   initialisation, the answer may be different for each path.
> should this be the US spelling 'initialization'?

Yes, we'd want to be consistent, indeed. Sometimes the British spelling
slips through as that's what I learned in high school.

Specifically for initialise:

$ git grep initialise
contrib/examples/git-notes.sh:                  die "Will not
initialise with empty tree"
object.h: * it can return "yes we have, and here is a half-initialised object"
object.h: * half-initialised objects, the caller is expected to initialize them
revision.c:static struct treesame_state *initialise_treesame(struct
rev_info *revs, struct commit *commit)
revision.c:                             ts = initialise_treesame(revs, commit);
+ a lot of french translations.

The American spelling is found a lot more.

Thanks,
Stefan