Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

>> But many callers do not follow that; rather they do
>>
>> loop to iterate over paths {
>> call a helper func to learn attr X for path
>> use the value of attr X
>> }
>>
>> using a callchain that embeds a helper function deep inside, and
>> "check" is kept in the helper, check-attr function is called from
>> there, and "result" is not passed from the caller to the helper
>> (obviously, because it does not exist in the current API).  See the
>> callchain that leads down to convert.c::convert_attrs() for a
>> typical example.  When converted to the new API, it needs to have a
>> new "result" structure every time it is called, and cannot reuse the
>> one that was used in its previous call.
>
> Why would that be? i.e. I do not understand the reasoning/motivation
> as well as what you propose to change here.

The leaf function may look like

check_eol(const char *path)
{
static struct git_attr_check *check;

initl(, "eol", NULL);
git_check_attr(, path, result);
return nth_element_in_result(result, 0);
}

and we want "result" to be thread-ready.  

A naive and undesired way to put it on stack is like so:

const char *check_eol(const char *path)
{
static struct git_attr_check *check;
struct git_attr_result result = RESULT_INIT;
const char *eol;

initl(, "eol", NULL);
init_result(, );
git_check_attr(, path, );
eol = nth_element_in_result(, 0);
clear_result();
return eol;
}

where your "struct git_attr_result" has a fixed size, and the actual
result array is allocated via ALLOC_GROW() etc.  That's overhead
that we do not want.  Instead can't we do this?

const char *check_eol(const char *path)
{
static struct git_attr_check *check;
/* we know we only want one */
struct git_attr_result result[1];
const char *eol;

initl(, "eol", NULL);
git_check_attr(, path, result);
return result[0];
}

That way, we won't be doing ALLOC_GROW() in init_result() or free in
clear_result().

If you use a structure that has pointer to an array (i.e. the "naive
and undesired way" above), you cannot amortize the alloc/free by
making result "static" if you want to be thread-ready.

Re: [PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

On Wed, Oct 12, 2016 at 2:45 PM, Jacob Keller  wrote:
> On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt  wrote:
>>
>> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
>> common knowledge among professionals that this naïve version _does_not_work_
>> [1]!
>>
>> I suggest you go without it, then measure, and only *then* optimize if it is
>> a bottleneck. Did I read "we do not expect much contention" somewhere?
>>
>> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
>> but applies to C just as well
>>
>> -- Hannes
>>
>
>
> You know, I always wondered why Linux Kernel code needed memory
> barriers but userspace programs didn't seem to use them.. turns out
> they actually *do* need them for the same exact types of problems...
>
> Thanks,
> Jake

In a former job I made use of them, too. So I am kinda embarrassed.
(I cannot claim I did not know about these patterns and memory
fencing, it just escaped my consciousness).

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano  wrote:
>> Johannes Sixt  writes:
>>
>>> Sigh. DCLP, the Double Checked Locking Pattern. ...
>>> I suggest you go without it, then measure, and only *then* optimize if
>>> it is a bottleneck.
>>
>> That comes from me in earlier discussion before the patch, namely in
>> , where I wondered if
>> a cheap check outside the lock may be a possible optimization
>> opportunity, as this is a classic singleton that will not be
>> deinitialized, and once the codepath gets exercised, we would be
>> taking the "nothing to do" route 100% of the time.
>
> Having followed that advice (and internally having a DCLP), I think
> we have Triple Checked Locking Pattern in this patch.  Nobody wrote
> a paper on how that would not work, yet. ;)
>
> In the reroll I plan to reduce it to a Single Checked (inside a mutex)
> Locking Pattern, though I would expect that performance (or lack thereof)
> will show then. But let's postpone measuring until we have a working patch.

Oh, that wasn't even an "advice" (read it again).  I fully agree
that starting simple would be the way to go.

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

>>> Well all of the hunks in the patch are not threaded, so they
>>> don't follow a threading pattern, but the static pattern to not be
>>> more expensive than needed.
>>
>> Is it too invasive a change to make them as if they are thread-ready
>> users of API that happen to know their callers are not threading?
>> It would be ideal if we can prepare them so that the way they
>> interact with the attr subsystem will not have to change after this
>> step.
>
> As far as I see the future, we do not need to change those in the future,
> unless we add the threading to the current callers, which is usually a very
> invasive thing?

It does not matter how invasive the thread set-up and teardown that
happens in the callers.

I am talking about the part of _THIS_ code that you are updating,
that interacts with attr API.  The way they prepare "check" and
"result", the way they ask questions by calling git_check_attr()
function.

Think of a thread-safe library function (like malloc()).  If you
write 

func (...) {
buf = malloc(20);
...
free(buf);
}

in a function that happens to be only called in a non-threaded
program today, you do not have to update these calls to malloc(3)
and free(3) when you update the callchain to threadable, right?

That kind of thread-preparedness is what I am trying to see if we
can achieve with this update.

Re: [PATCHv2] attr: convert to new threadsafe API

[Jacob Keller]

On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt  wrote:
>
> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
> common knowledge among professionals that this naïve version _does_not_work_
> [1]!
>
> I suggest you go without it, then measure, and only *then* optimize if it is
> a bottleneck. Did I read "we do not expect much contention" somewhere?
>
> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
> but applies to C just as well
>
> -- Hannes
>


You know, I always wondered why Linux Kernel code needed memory
barriers but userspace programs didn't seem to use them.. turns out
they actually *do* need them for the same exact types of problems...

Thanks,
Jake

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> This version adds the actual thread safety,
>>> that is promised in the documentation, however it doesn't add any 
>>> optimization,
>>> i.e. it's a single global lock. But as we do not expect contention, that is 
>>> fine.
>>
>> Because we have to start _somewhere_, I agree it is a good approach
>> to first try the simplest implementation and then optimize later,
>> but is it an agreed consensus that we do not expect contention?
>
> I agree on that. Did you mean this is obvious to the reader?

I meant to say that "But as we do not expect" sounded like a
justification for the approach based on an unwarranted assumption
that is not even the list concensus.  I do not think it is obvious
to the reader that there is no need to worry about contention.

It all is outside the log message, so as long as readers understand
what we meant from this discussion, that is OK.

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Johannes Sixt  writes:

> Sigh. DCLP, the Double Checked Locking Pattern. ...
> I suggest you go without it, then measure, and only *then* optimize if
> it is a bottleneck. 

That comes from me in earlier discussion before the patch, namely in
, where I wondered if
a cheap check outside the lock may be a possible optimization
opportunity, as this is a classic singleton that will not be
deinitialized, and once the codepath gets exercised, we would be
taking the "nothing to do" route 100% of the time.

Re: [PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano  wrote:
> Johannes Sixt  writes:
>
>> Sigh. DCLP, the Double Checked Locking Pattern. ...
>> I suggest you go without it, then measure, and only *then* optimize if
>> it is a bottleneck.
>
> That comes from me in earlier discussion before the patch, namely in
> , where I wondered if
> a cheap check outside the lock may be a possible optimization
> opportunity, as this is a classic singleton that will not be
> deinitialized, and once the codepath gets exercised, we would be
> taking the "nothing to do" route 100% of the time.
>

Having followed that advice (and internally having a DCLP), I think
we have Triple Checked Locking Pattern in this patch.  Nobody wrote
a paper on how that would not work, yet. ;)

In the reroll I plan to reduce it to a Single Checked (inside a mutex)
Locking Pattern, though I would expect that performance (or lack thereof)
will show then. But let's postpone measuring until we have a working patch.

Re: [PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This version adds the actual thread safety,
>> that is promised in the documentation, however it doesn't add any 
>> optimization,
>> i.e. it's a single global lock. But as we do not expect contention, that is 
>> fine.
>
> Because we have to start _somewhere_, I agree it is a good approach
> to first try the simplest implementation and then optimize later,
> but is it an agreed consensus that we do not expect contention?
>

I agree on that. Did you mean this is obvious to the reader?  I just wanted
to state how I'll do it, such that if in the future someone digs up
this code, they'll
know I did not look at performance in the first run.

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

> This version adds the actual thread safety,
> that is promised in the documentation, however it doesn't add any 
> optimization,
> i.e. it's a single global lock. But as we do not expect contention, that is 
> fine.

Because we have to start _somewhere_, I agree it is a good approach
to first try the simplest implementation and then optimize later,
but is it an agreed consensus that we do not expect contention?

Re: [PATCHv2] attr: convert to new threadsafe API

[Johannes Sixt]

Am 12.10.2016 um 01:59 schrieb Stefan Beller:

+void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
 {
-   struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+   struct git_attr_check *check;
+
+   if (*check_)
+   return;
+
+   attr_lock();
+   if (*check_) {
+   attr_unlock();
+   return;
+   }
...
check = xcalloc(1,
-   sizeof(*check) + cnt * sizeof(*(check->check)));
+   sizeof(*check) + cnt * sizeof(*(check->attr)));
...
+   *check_ = check;
+   attr_unlock();


Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be 
common knowledge among professionals that this naïve version 
_does_not_work_ [1]!


I suggest you go without it, then measure, and only *then* optimize if 
it is a bottleneck. Did I read "we do not expect much contention" somewhere?


[1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ 
centric, but applies to C just as well


-- Hannes

Re: [PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

On Wed, Oct 12, 2016 at 9:20 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> I am not sure if the updates to the callers fulfill that purpose.
>>> For example, look at this hunk.
>>>
 @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char 
 *sha1, const char *base,
   struct archiver_args *args = c->args;
   write_archive_entry_fn_t write_entry = c->write_entry;
   static struct git_attr_check *check;
 + static struct git_attr_result result;
>>>
>>> As we discussed, this caller, even when threaded, will always want
>>> to ask for a fixed two attributes, so "check" being static and
>>> shared across threads is perfectly fine.  But we do not want to see
>>> "result" shared, do we?
>>
>> Well all of the hunks in the patch are not threaded, so they
>> don't follow a threading pattern, but the static pattern to not be
>> more expensive than needed.
>
> Is it too invasive a change to make them as if they are thread-ready
> users of API that happen to know their callers are not threading?
> It would be ideal if we can prepare them so that the way they
> interact with the attr subsystem will not have to change after this
> step.

As far as I see the future, we do not need to change those in the future,
unless we add the threading to the current callers, which is usually a very
invasive thing?

>
>>> In other words, ideally, I think this part of the patch should
>>> rather read like this:
>>>
>>> static struct git_attr_check *check;
>>> struct git_attr_result result[2];
>>>
>>> ...
>>> git_attr_check_initl(, "export-ignore", "export-subst", NULL);
>>> if (!git_check_attr(path_without_prefix, check, result)) {
>>> ... use result[0] and result[1] ...
>>>
>>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>>> and extra parameter to git_check_attr() so that the function can
>>> make sure it matches (or exceeds) check->nr.
>>
>> That seems tempting from a callers perspective; I'll look into that.
>
> For callers that prepare "check" and "result" before asking
> check-attr about the attributes in "check" for many paths, it is OK
> to use your "allocate with attr_result_init()" pattern.  The "result"
> still needs to be made non-static, though.

I do not see why we would want to have a non static result for
non threaded callers.

>
> But many callers do not follow that; rather they do
>
> loop to iterate over paths {
> call a helper func to learn attr X for path
> use the value of attr X
> }
>
> using a callchain that embeds a helper function deep inside, and
> "check" is kept in the helper, check-attr function is called from
> there, and "result" is not passed from the caller to the helper
> (obviously, because it does not exist in the current API).  See the
> callchain that leads down to convert.c::convert_attrs() for a
> typical example.  When converted to the new API, it needs to have a
> new "result" structure every time it is called, and cannot reuse the
> one that was used in its previous call.

Why would that be? i.e. I do not understand the reasoning/motivation
as well as what you propose to change here. Do you think of the result
being held at the top of the call chain (and there it may be just allocated
on the stack) and passed down all the way to convert_attrs, that writes
into that result?

Currently the static result in convert_attrs just works fine (as all tests pass)
and that is not the place where we'd add threading to?

Threading would be added in dir.c, a new place that will use attrs.

Maybe instead of discussing the meaning of this patch further, I should
rebase the "pathspecs can ask for attributes" patches to either experience
the problems myself or at least have a series that has a meaningful endgoal.
I thought this preparatory series may be better than sending an even
larger series.

Thanks,
Stefan

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

>> I am not sure if the updates to the callers fulfill that purpose.
>> For example, look at this hunk.
>>
>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char 
>>> *sha1, const char *base,
>>>   struct archiver_args *args = c->args;
>>>   write_archive_entry_fn_t write_entry = c->write_entry;
>>>   static struct git_attr_check *check;
>>> + static struct git_attr_result result;
>>
>> As we discussed, this caller, even when threaded, will always want
>> to ask for a fixed two attributes, so "check" being static and
>> shared across threads is perfectly fine.  But we do not want to see
>> "result" shared, do we?
> 
> Well all of the hunks in the patch are not threaded, so they
> don't follow a threading pattern, but the static pattern to not be
> more expensive than needed.

Is it too invasive a change to make them as if they are thread-ready
users of API that happen to know their callers are not threading?
It would be ideal if we can prepare them so that the way they
interact with the attr subsystem will not have to change after this
step.

>> In other words, ideally, I think this part of the patch should
>> rather read like this:
>>
>> static struct git_attr_check *check;
>> struct git_attr_result result[2];
>>
>> ...
>> git_attr_check_initl(, "export-ignore", "export-subst", NULL);
>> if (!git_check_attr(path_without_prefix, check, result)) {
>> ... use result[0] and result[1] ...
>>
>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>> and extra parameter to git_check_attr() so that the function can
>> make sure it matches (or exceeds) check->nr.
>
> That seems tempting from a callers perspective; I'll look into that.

For callers that prepare "check" and "result" before asking
check-attr about the attributes in "check" for many paths, it is OK
to use your "allocate with attr_result_init()" pattern.  The "result"
still needs to be made non-static, though.

But many callers do not follow that; rather they do

loop to iterate over paths {
call a helper func to learn attr X for path
use the value of attr X
}

using a callchain that embeds a helper function deep inside, and
"check" is kept in the helper, check-attr function is called from
there, and "result" is not passed from the caller to the helper
(obviously, because it does not exist in the current API).  See the
callchain that leads down to convert.c::convert_attrs() for a
typical example.  When converted to the new API, it needs to have a
new "result" structure every time it is called, and cannot reuse the
one that was used in its previous call.

Re: [PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

On Tue, Oct 11, 2016 at 11:12 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> I think this patch is the most interesting patch, so I'll refrain from
>> resending the other 27 patches, though I have adressed the review comments
>> locally. I'll resend everything once we are in agreement for this one.
>
> What is the primary purpose of this patch?  Is it to prepare callers
> so that the way they interact with the attr subsystem will not have to
> change when they become threaded and the attr subsystem becomes
> thread ready?
>
> I am not sure if the updates to the callers fulfill that purpose.
> For example, look at this hunk.
>
>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char 
>> *sha1, const char *base,
>>   struct archiver_args *args = c->args;
>>   write_archive_entry_fn_t write_entry = c->write_entry;
>>   static struct git_attr_check *check;
>> + static struct git_attr_result result;
>
> As we discussed, this caller, even when threaded, will always want
> to ask for a fixed two attributes, so "check" being static and
> shared across threads is perfectly fine.  But we do not want to see
> "result" shared, do we?

Well all of the hunks in the patch are not threaded, so they
don't follow a threading pattern, but the static pattern to not be
more expensive than needed.

>
>>   const char *path_without_prefix;
>>   int err;
>>
>> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char 
>> *sha1, const char *base,
>>   strbuf_addch(, '/');
>>   path_without_prefix = path.buf + args->baselen;
>>
>> - if (!check)
>> - check = git_attr_check_initl("export-ignore", "export-subst", 
>> NULL);
>> - if (!git_check_attr(path_without_prefix, check)) {
>> - if (ATTR_TRUE(check->check[0].value))
>> + if (!check) {
>> + git_attr_check_initl(, "export-ignore", "export-subst", 
>> NULL);
>> + git_attr_result_init(, check);
>> + }
>
> Are we assuming that storing and checking of a single pointer is
> atomic?  I would not expose that assumption to the callers.  On a
> platform where that assumption holds, "if check is not NULL,
> somebody must have done it already, so return without doing nothing"
> can be the first thing git_attr_check_initl()'s implementation does,
> though.  Or it may not hold anywhere without some barriers.  All
> that implementation details should be hidden inside _initl()'s
> implementation.  So this caller should instead just do an
> unconditional:
>
> git_attr_check_initl(, "export-ignore", "export-subst", NULL);
>
> Also, as "result" should be per running thread, hence non-static,
> and because we do not want repeated heap allocations and releases
> but luckily most callers _know_ not just how many but what exact
> attributes they are interested in (I think there are only two
> callers that do not know it; check-all-attrs one, and your pathspec
> magic one that does not exist at this point in the series), I would
> think it is much more preferrable to allow the caller to prepare an
> on-stack array and call it "initialized already".
>
> In other words, ideally, I think this part of the patch should
> rather read like this:
>
> static struct git_attr_check *check;
> struct git_attr_result result[2];
>
> ...
> git_attr_check_initl(, "export-ignore", "export-subst", NULL);
> if (!git_check_attr(path_without_prefix, check, result)) {
> ... use result[0] and result[1] ...
>
> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
> and extra parameter to git_check_attr() so that the function can
> make sure it matches (or exceeds) check->nr.

That seems tempting from a callers perspective; I'll look into that.

Re: [PATCHv2] attr: convert to new threadsafe API

[Junio C Hamano]

Stefan Beller  writes:

> I think this patch is the most interesting patch, so I'll refrain from
> resending the other 27 patches, though I have adressed the review comments
> locally. I'll resend everything once we are in agreement for this one.

What is the primary purpose of this patch?  Is it to prepare callers
so that the way they interact with the attr subsystem will not have to
change when they become threaded and the attr subsystem becomes
thread ready?

I am not sure if the updates to the callers fulfill that purpose.
For example, look at this hunk.

> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, 
> const char *base,
>   struct archiver_args *args = c->args;
>   write_archive_entry_fn_t write_entry = c->write_entry;
>   static struct git_attr_check *check;
> + static struct git_attr_result result;

As we discussed, this caller, even when threaded, will always want
to ask for a fixed two attributes, so "check" being static and
shared across threads is perfectly fine.  But we do not want to see
"result" shared, do we?

>   const char *path_without_prefix;
>   int err;
>  
> @@ -124,12 +125,15 @@ static int write_archive_entry(const unsigned char 
> *sha1, const char *base,
>   strbuf_addch(, '/');
>   path_without_prefix = path.buf + args->baselen;
>  
> - if (!check)
> - check = git_attr_check_initl("export-ignore", "export-subst", 
> NULL);
> - if (!git_check_attr(path_without_prefix, check)) {
> - if (ATTR_TRUE(check->check[0].value))
> + if (!check) {
> + git_attr_check_initl(, "export-ignore", "export-subst", 
> NULL);
> + git_attr_result_init(, check);
> + }

Are we assuming that storing and checking of a single pointer is
atomic?  I would not expose that assumption to the callers.  On a
platform where that assumption holds, "if check is not NULL,
somebody must have done it already, so return without doing nothing"
can be the first thing git_attr_check_initl()'s implementation does,
though.  Or it may not hold anywhere without some barriers.  All
that implementation details should be hidden inside _initl()'s
implementation.  So this caller should instead just do an
unconditional:

git_attr_check_initl(, "export-ignore", "export-subst", NULL);

Also, as "result" should be per running thread, hence non-static,
and because we do not want repeated heap allocations and releases
but luckily most callers _know_ not just how many but what exact
attributes they are interested in (I think there are only two
callers that do not know it; check-all-attrs one, and your pathspec
magic one that does not exist at this point in the series), I would
think it is much more preferrable to allow the caller to prepare an
on-stack array and call it "initialized already".  

In other words, ideally, I think this part of the patch should
rather read like this:

static struct git_attr_check *check;
struct git_attr_result result[2];

...
git_attr_check_initl(, "export-ignore", "export-subst", NULL);
if (!git_check_attr(path_without_prefix, check, result)) {
... use result[0] and result[1] ...

For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
and extra parameter to git_check_attr() so that the function can
make sure it matches (or exceeds) check->nr.

[PATCHv2] attr: convert to new threadsafe API

[Stefan Beller]

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 check;

if (!check)
check = git_attr_check_initl("text", NULL);

git_check_attr(path, check);
act_on(check->value[0]);

This has a couple of issues when it comes to thread safety:

* the initialization is racy in this implementation. To make it
  thread safe, we need to acquire a mutex, such that only one
  thread is executing the code in git_attr_check_initl.
  As we do not want to introduce a mutex at each call site,
  this is best done in the attr code. However to do so, we need
  to have access to the `check` variable, i.e. the API has to
  look like
git_attr_check_initl(struct git_attr_check*, ...);
  Then one of the threads calling git_attr_check_initl will
  acquire the mutex and init the `check`, while all other threads
  will wait on the mutex just to realize they're late to the
  party and they'll return with no work done.

* 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.

This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety.

Signed-off-by: Stefan Beller 
---

I think this patch is the most interesting patch, so I'll refrain from
resending the other 27 patches, though I have adressed the review comments
locally. I'll resend everything once we are in agreement for this one.

This version adds the actual thread safety,
that is promised in the documentation, however it doesn't add any optimization,
i.e. it's a single global lock. But as we do not expect contention, that is 
fine.

Also there is no example of how to use the thread safe API for asking questions
from multiple threads.

Thanks,
Stefan

 Documentation/technical/api-gitattributes.txt |  93 +--
 archive.c |  14 ++-
 attr.c| 128 +-
 attr.h|  80 ++--
 builtin/check-attr.c  |  30 +++---
 builtin/pack-objects.c|  12 ++-
 convert.c |  35 +++
 ll-merge.c|  27 --
 userdiff.c|  19 ++--
 ws.c  |  15 +--
 10 files changed, 292 insertions(+), 161 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 92fc32a..ac97244 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -8,6 +8,18 @@ attributes to set of paths.
 Data Structure
 --
 
+extern struct git_attr *git_attr(const char *);
+
+/*
+ * Return the name of the attribute represented by the argument.  The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
 `struct git_attr`::
 
An attribute is an opaque object that is identified by its name.
@@ -16,15 +28,17 @@ Data Structure
of no interest to the calling programs.  The name of the
attribute can be retrieved by calling `git_attr_name()`.
 
-`struct git_attr_check_elem`::
-
-   This structure represents one attribute and its value.
-
 `struct git_attr_check`::
 
-   This structure represents a collection of `git_attr_check_elem`.
+   This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
-   attributes to check, and receives their values.
+   attributes to check, and receives their values into a corresponding
+   `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+   This structure represents a collection of results to its
+   corresponding `struct git_attr_check`, that has the same order.
 
 
 Attribute Values
@@ -53,19 +67,30 @@ value of the attribute for the path.
 Querying Specific Attributes
 
 
-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
   function, enumerating the names of attributes whose values you are
   interested in, terminated with a NULL pointer.  Alternatively, an
-  empty `struct git_attr_check` can be prepared by