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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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*

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

2016-10-12 Thread 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 >>>

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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 =

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

2016-10-12 Thread 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

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

2016-10-12 Thread 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

[PATCHv2] attr: convert to new threadsafe API

2016-10-11 Thread 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,