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

On Mon, Oct 3, 2016 at 1:49 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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;

quoted

    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.