Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)

From: Stefan Beller <sbeller@google.com>
Date: Oct 3, 2016, 21:49 UTC
Message-ID: <CAGZ79kYt+Z=ff1b2G+wWRAGGS=je+dpksfmMXj0fWwYVvHk8Cg@mail.gmail.com>
In-Reply-To: <xmqqwphp5f9q.fsf@gitster.mtv.corp.google.com>

On Mon, Oct 3, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> I am looking at the tip of jc/attr-more and for a minimum
>> thread safety we'd want to change the call sites to be aware of the
>> threads, i.e. instead of doing
>>
>       static struct git_attr_check *check;
>>     if (!check)
>>         check = git_attr_check_initl("crlf", "ident",
>>                     "filter", "eol", "text",
>>                     NULL);
>>
>> We'd rather call
>>
>>         struct git_attr_check *check;
>>         check = git_attr_check_lookup_or_initl_threadsafe(
>>                 "crlf", "ident", "filter", "eol", "text", NULL);
>>          if (!git_check_attr(path, check)) {
>>              ...
>
> I actually am hoping that the "static" can be kept so that we can
> minimize the cost of interning these symbols into struct git_attr.
>
> The initialization would thus become something like:
>
>         static struct git_attr_check *check;
>         git_attr_check_initl(&check, "crlf", "ident", ..., NULL);
>
> The structure will have an array of git_attr[], once interned will
> be shared and used by everybody.  _initl() will need to make sure
> that the "check" pointer is updated atomically so that multiple
> people racing to initialize this variable will not step on each
> others' toes.

I see and that mutex to protect the first argument of git_attr_check_initl is unrelated to the actual pointer; we can use a global mutex for that, or rather a static scoped mutex in attr.c, such that all parallel racing git_attr_check_initl try to acquire that init_lock and only one succeeds and that one makes sure to first initialize a git_attr_check completely and then atomically storing the pointer to the &check location.

>
> Then the use site would do something like
>
>         const char *result[... some size ...];

... some size ... depends on the git_attr_check->check_nr and not on the path as I first assumed. So when we still want to go fast here:

    static struct git_attr_check *check;

    /* hard code 2 as it has to hold results for crlf and ident only */
    const char *results[2];

    if (!check)
        git_attr_check_initl(&check, "crlf", "ident", NULL);

    if (nr != check->check_nr)
        ALLOC_GROW(results, check->check_nr, alloc);

    git_check_attr(path, check, &result);
    // result[i] contains the result for the i-th element of check

    // Note: git_attr_check_elem seems to be useless now, as the
    // results are not stored in there, we only make use of the `attr` key.