Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Johannes Sixt writes: > Am 28.10.2016 um 22:29 schrieb Junio C Hamano: >> Johannes Sixt writes: >> >>> Another problem with the proposed patch is that there is no >>> declaration for attr_start() before the call in compat/mingw.c. We >>> would have to move the declaration of attr_start() to cache.h (for >>> example), because #including attr.h in compat/mingw.c is plainly >>> wrong. However, it would not be a major offense to #include attr.h in >>> common-main.c. But when we do that, we can certainly spare the few >>> cycles to call pthread_mutex_init. >> >> That sounds like a good argument to have it in common-main.c. >> >> Would it mean that the code that defines the mutex needs to have >> #ifdef that defines a no-op attr_start() and defines the mutex with >> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex >> without initializatin, with the real attr_start(), though? > > No. My intent was to call pthread_mutex_init for all platforms. Ah, OK, so there was no magic plan. That's OK.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 28.10.2016 um 22:29 schrieb Junio C Hamano:
Johannes Sixt writes:
Another problem with the proposed patch is that there is no
declaration for attr_start() before the call in compat/mingw.c. We
would have to move the declaration of attr_start() to cache.h (for
example), because #including attr.h in compat/mingw.c is plainly
wrong. However, it would not be a major offense to #include attr.h in
common-main.c. But when we do that, we can certainly spare the few
cycles to call pthread_mutex_init.
That sounds like a good argument to have it in common-main.c.
Would it mean that the code that defines the mutex needs to have
#ifdef that defines a no-op attr_start() and defines the mutex with
PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex
without initializatin, with the real attr_start(), though?
No. My intent was to call pthread_mutex_init for all platforms. We
already call open("/dev/null") unconditionally on every program startup.
The few cycles spent in pthread_mutex_init should be the least of our
worries.
-- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Stefan Beller writes: > On Fri, Oct 28, 2016 at 1:29 PM, Junio C Hamano wrote: >> Johannes Sixt writes: >> >>> Another problem with the proposed patch is that there is no >>> declaration for attr_start() before the call in compat/mingw.c. We >>> would have to move the declaration of attr_start() to cache.h (for >>> example), because #including attr.h in compat/mingw.c is plainly >>> wrong. However, it would not be a major offense to #include attr.h in >>> common-main.c. But when we do that, we can certainly spare the few >>> cycles to call pthread_mutex_init. >> >> That sounds like a good argument to have it in common-main.c. > > If we're going that route, I would get rid of PTHREAD_MUTEX_INITILIZER > and call a pthread_mutex_init platform independently. Yup, but earlier j6t was trying hard not to penalize any single platform, and that would certainly penalize the ones with static initialization, so I was hoping to hear if there is a clever workaround to avoid that. >> Would it mean that the code that defines the mutex needs to have >> #ifdef that defines a no-op attr_start() and defines the mutex with >> PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex >> without initializatin, with the real attr_start(), though?
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Fri, Oct 28, 2016 at 1:29 PM, Junio C Hamano wrote: > Johannes Sixt writes: > >> Another problem with the proposed patch is that there is no >> declaration for attr_start() before the call in compat/mingw.c. We >> would have to move the declaration of attr_start() to cache.h (for >> example), because #including attr.h in compat/mingw.c is plainly >> wrong. However, it would not be a major offense to #include attr.h in >> common-main.c. But when we do that, we can certainly spare the few >> cycles to call pthread_mutex_init. > > That sounds like a good argument to have it in common-main.c. If we're going that route, I would get rid of PTHREAD_MUTEX_INITILIZER and call a pthread_mutex_init platform independently. > > Would it mean that the code that defines the mutex needs to have > #ifdef that defines a no-op attr_start() and defines the mutex with > PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex > without initializatin, with the real attr_start(), though?
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Johannes Sixt writes: > Another problem with the proposed patch is that there is no > declaration for attr_start() before the call in compat/mingw.c. We > would have to move the declaration of attr_start() to cache.h (for > example), because #including attr.h in compat/mingw.c is plainly > wrong. However, it would not be a major offense to #include attr.h in > common-main.c. But when we do that, we can certainly spare the few > cycles to call pthread_mutex_init. That sounds like a good argument to have it in common-main.c. Would it mean that the code that defines the mutex needs to have #ifdef that defines a no-op attr_start() and defines the mutex with PTHREAD_MUTEX_INITILIZER with #else that just defines the mutex without initializatin, with the real attr_start(), though?
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 28.10.2016 um 20:48 schrieb Jacob Keller: On Fri, Oct 28, 2016 at 4:58 AM, Johannes Schindelin wrote: Hi Jake, On Thu, 27 Oct 2016, Jacob Keller wrote: I agree with Stefan that there isn't really a great place to put a dynamic initialization. Ummm. Wait. What??? https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41 I think Stefan has since provided a better suggestion of isolating the dynamic initialization into the MINGW startup section. However, you are right either place works, though I think platforms which support static initialization should use it. It's a pity that this new patch introduces the first use of PTHREAD_MUTEX_INITILIZER. (I do not count the one in compat/nedmalloc, it's in platform specific code.) We have to introduce a dummy definition on Windows to even compile the code. Now we have a double-edged sword: Other coders who are not aware of this thread might think it works. But it does not. Another problem with the proposed patch is that there is no declaration for attr_start() before the call in compat/mingw.c. We would have to move the declaration of attr_start() to cache.h (for example), because #including attr.h in compat/mingw.c is plainly wrong. However, it would not be a major offense to #include attr.h in common-main.c. But when we do that, we can certainly spare the few cycles to call pthread_mutex_init. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Fri, Oct 28, 2016 at 6:01 AM, Philip Oakley wrote: > From: "Johannes Sixt" >> >> >> One point is that the DCLP idiom must be implemented correctly. There are >> solutions, of course, and when the initialization is over, we have a >> miniscule overhead at each pthread_mutex_lock call. >> > > I had to look up DCLP ( = Double Checked Locking Patterns), and found a good > write up on the issues.. > > http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf "C++ and the > Perils of Double-Checked Locking", which include 'C' issues, and > multi-thread, multi-processor issues. Not an easy issue when fighting > optimisers.. > > -- > > Philip Yep, this is why we have memory barriers. Ofcourse languages like C don't really allow you to express them in the language and we restore to various platform specific methods. Thanks, Jake
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Fri, Oct 28, 2016 at 4:58 AM, Johannes Schindelin wrote: > Hi Jake, > > On Thu, 27 Oct 2016, Jacob Keller wrote: > >> I agree with Stefan that there isn't really a great place to put a >> dynamic initialization. > > Ummm. Wait. What??? > > https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41 > > Ciao, > Johannes I think Stefan has since provided a better suggestion of isolating the dynamic initialization into the MINGW startup section. However, you are right either place works, though I think platforms which support static initialization should use it. Thanks, Jake
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: "Johannes Sixt" One point is that the DCLP idiom must be implemented correctly. There are solutions, of course, and when the initialization is over, we have a miniscule overhead at each pthread_mutex_lock call. I had to look up DCLP ( = Double Checked Locking Patterns), and found a good write up on the issues.. http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf "C++ and the Perils of Double-Checked Locking", which include 'C' issues, and multi-thread, multi-processor issues. Not an easy issue when fighting optimisers.. -- Philip
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Hi Jake, On Thu, 27 Oct 2016, Jacob Keller wrote: > I agree with Stefan that there isn't really a great place to put a > dynamic initialization. Ummm. Wait. What??? https://github.com/git/git/blob/v2.10.1/common-main.c#L25-L41 Ciao, Johannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Thu, Oct 27, 2016 at 10:55 PM, Johannes Sixt wrote: > One point is that the DCLP idiom must be implemented correctly. There are > solutions, of course, and when the initialization is over, we have a > miniscule overhead at each pthread_mutex_lock call. > Right, this I understood, but appeared to be solved. > The main point is that the initialization has to solve a chicken-and-egg > problem: After we have found an uninitialized critical section, we have to > have mutual exclusion for the initialization. We need another critical > section for this, but we cannot have one that is initialized. For this > reason, the solution uses a different kind of mutual exclusion primitive, > which is more akin to POSIX semaphores and works across processes. In the > patch proposed by Stefan, a *session-wide* mutex is used. That means that > all concurrent git invocations in a user's session synchronize their > initialization of critical section objects. > Thank you for explaining this. Now I understand why this would be considered a big issue, and potentially worth considering the alternatives like attr_start. This was missing since I don't think any of the rest of us knew (correct me if I am wrong) that the synchronization would be global. For many cases it's probably not that bad, but we do have a suitable explanation, and I think living with "attr_start()" in the win32 initialization path which is something Stefan suggested in a previous email would make the most sense then. > That's just ridiculous. It's like waiting for a ... no, *the* ... battle > ship just to get our bouncers in their position. We are talking milliseconds > here, not nanoseconds. > Right. Thanks for filling in the missing details. Regards, Jake > -- Hannes >
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 27.10.2016 um 23:49 schrieb Jacob Keller: Ok, so I've been reading this thread. I don't understand your objections to emulating in this way.. Could you clearly spell out why you believe this solution isn't acceptable? So far all I've understood was "it's not critical sections" and "it penalizes Windows too much" but... If Windows cannot statically initialize a pthread mutex, then we *have* to dynamically initialize it somewhere. This solution adds a single check before each lock and is safe due to use of memory barriers. Yes, this will cost a tiny bit extra overhead for each use of "pthread_mutex_lock" but I fail to see how that is a huge penalty... One point is that the DCLP idiom must be implemented correctly. There are solutions, of course, and when the initialization is over, we have a miniscule overhead at each pthread_mutex_lock call. The main point is that the initialization has to solve a chicken-and-egg problem: After we have found an uninitialized critical section, we have to have mutual exclusion for the initialization. We need another critical section for this, but we cannot have one that is initialized. For this reason, the solution uses a different kind of mutual exclusion primitive, which is more akin to POSIX semaphores and works across processes. In the patch proposed by Stefan, a *session-wide* mutex is used. That means that all concurrent git invocations in a user's session synchronize their initialization of critical section objects. That's just ridiculous. It's like waiting for a ... no, *the* ... battle ship just to get our bouncers in their position. We are talking milliseconds here, not nanoseconds. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
> there isn't really a great place to put a dynamic initialization. Answering this question specifically (Where to put it?), I am about to send out a patch that puts it in compat/mingw.c:2232: diff --git a/compat/mingw.c b/compat/mingw.c index 3fbfda5..9881c3d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2232,6 +2232,9 @@ void mingw_startup(void) /* initialize critical section for waitpid pinfo_t list */ InitializeCriticalSection(&pinfo_cs); +/* initialize critical sections in the attr code */ +start_attr(); + /* set up default file mode and file modes for stdin/out/err */ _fmode = _O_BINARY; _setmode(_fileno(stdin), _O_BINARY); If I understood Johannes correctly this is the place to put it. Junio seems to be ok with static mutex init for non windows platforms, so I put it into the mingw specifc startup code. Thanks, Stefan
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Thu, Oct 27, 2016 at 1:05 PM, Johannes Sixt wrote: >> The implementation under discussion (well we did not discuss the >> implementation a >> whole lot yet) ... > > > There's not a whole lot to discuss: it must be rewritten from scratch (it's > not just the memory barriers, it is everything else, too). But time is much > better spent on an attr_start() solution. > > -- Hannes > Ok, so I've been reading this thread. I don't understand your objections to emulating in this way.. Could you clearly spell out why you believe this solution isn't acceptable? So far all I've understood was "it's not critical sections" and "it penalizes Windows too much" but... If Windows cannot statically initialize a pthread mutex, then we *have* to dynamically initialize it somewhere. This solution adds a single check before each lock and is safe due to use of memory barriers. Yes, this will cost a tiny bit extra overhead for each use of "pthread_mutex_lock" but I fail to see how that is a huge penalty... So I'm trying to understand if that really is your only concern, because I don't actually buy that it is a huge overhead to pay. I agree with Stefan that there isn't really a great place to put a dynamic initialization. To be clear I am trying to understand the objections since so far all I have read is "this is bad, and I object" without clearly explaining reasoning. Regards, Jake
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 27.10.2016 um 21:08 schrieb Stefan Beller: On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano wrote: Johannes Sixt writes: Am 27.10.2016 um 19:01 schrieb Stefan Beller: ... It is not possible to mark a mutex uninitialized on Windows without an extra piece of data. A solution would become quite complicated quite quickly, and at the cost of additional operations that are in the same ballpark as an uncontended mutex. I'm not enthused. The positive aspect of this way the patch proposes would be that any future contributor not knowing the details of how to do mutexes right on Windows, would not totally break the whole system, i.e. this seems to be more maintainable in the future as it reduces the friction between pthreads mutexes and the way we can do things in Git in a platform independent way This is a non-argument. Coders have to know their tools. Windows is not my tool. The tool I meant is pthreads. For example, you can't have a pthread_mutex_t variable and not initialize it with either PTHREAD_MUTEX_INITIALIZER or pthread_mutex_init. The codebase should strive to give coders a coherent abstraction that can be implemented efficiently on platforms, so that coders do not have to care too deeply about quirks that exist on individual platforms. Currently working as a coder I care about "submodules, that work on linux." I do not care about Windows in the big picture. I don't know what you meant to say with this sentence, but taken literally, are you on the right ship here, I have to ask? I am however willing to go an extra step to not break Windows. "Not break Windows" is equivalent to "make it work on Windows", mind you. We can't have a new feature only on Linux when there is no reason not to have it on Windows as well. Sorry, "Git is for Unix only" is long over. However that requires a little bit of effort from both me and you: * I need to be aware of what I cannot do with "not-my-tools". (So please somebody tell me, also in the future when I break obscure platforms. Mind that I don't equate obscure with not widely used or in any other way negative. It's just that people working on linux find some quirks on Windows not "obvious") That goes without saying. * the workaround should not be too time consuming in the bigger picture, That, however, is wishful thinking. If you want to have a feature dearly, you have to make it work, and that may take its time. I'm not saying that *you* have to make it work (there are platform experts around to lend a hand), but just an extra step to "not break" an important platform is not enough. which is why I propose to make the API a bit clearer by emulating posix mutexes harder. (From a Windows POV this might sound like making it more obscure because posix mutexes itself are obscure.) So, when you think that POSIX mutexes are obscure, why don't we settle on the simpler Windows critical sections? Emulating them with pthreads should be child's play. The implementation under discussion (well we did not discuss the implementation a whole lot yet) ... There's not a whole lot to discuss: it must be rewritten from scratch (it's not just the memory barriers, it is everything else, too). But time is much better spent on an attr_start() solution. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano wrote: > Johannes Sixt writes: > >> Am 27.10.2016 um 19:01 schrieb Stefan Beller: >> ... >> It is not possible to mark a mutex uninitialized on Windows without an >> extra piece of data. A solution would become quite complicated quite >> quickly, and at the cost of additional operations that are in the same >> ballpark as an uncontended mutex. I'm not enthused. >> >>> The positive aspect of this way the patch proposes would be that any >>> future contributor not knowing the details of how to do mutexes right >>> on Windows, would not totally break the whole system, i.e. this seems >>> to be more maintainable in the future as it reduces the friction between >>> pthreads mutexes and the way we can do things in Git in a platform >>> independent way >> >> This is a non-argument. Coders have to know their tools. Windows is not my tool. > > The codebase should strive to give coders a coherent abstraction > that can be implemented efficiently on platforms, so that coders do > not have to care too deeply about quirks that exist on individual > platforms. Currently working as a coder I care about "submodules, that work on linux." I do not care about Windows in the big picture. I am however willing to go an extra step to not break Windows. However that requires a little bit of effort from both me and you: * I need to be aware of what I cannot do with "not-my-tools". (So please somebody tell me, also in the future when I break obscure platforms. Mind that I don't equate obscure with not widely used or in any other way negative. It's just that people working on linux find some quirks on Windows not "obvious") * the workaround should not be too time consuming in the bigger picture, which is why I propose to make the API a bit clearer by emulating posix mutexes harder. (From a Windows POV this might sound like making it more obscure because posix mutexes itself are obscure.) > > It is OK to argue that the particular solution Stefan lifted from > somewhere (perhaps msdn article he cited???) A stack overflow article that I found with my search engine of choice, because I could not believe that Windows cannot have statically initialized mutexes. > does not qualify as > such an abstraction. The implementation under discussion (well we did not discuss the implementation a whole lot yet) may even contain an error as the first memory barrier needs to be in front of the first condition. > But I do not agree with your "Coders have to > know" as a blanket statement. Well I do to some extent, I just disagree what my tools are.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Johannes Sixt writes: > Am 27.10.2016 um 19:01 schrieb Stefan Beller: > ... > It is not possible to mark a mutex uninitialized on Windows without an > extra piece of data. A solution would become quite complicated quite > quickly, and at the cost of additional operations that are in the same > ballpark as an uncontended mutex. I'm not enthused. > >> The positive aspect of this way the patch proposes would be that any >> future contributor not knowing the details of how to do mutexes right >> on Windows, would not totally break the whole system, i.e. this seems >> to be more maintainable in the future as it reduces the friction between >> pthreads mutexes and the way we can do things in Git in a platform >> independent way > > This is a non-argument. Coders have to know their tools. The codebase should strive to give coders a coherent abstraction that can be implemented efficiently on platforms, so that coders do not have to care too deeply about quirks that exist on individual platforms. It is OK to argue that the particular solution Stefan lifted from somewhere (perhaps msdn article he cited???) does not qualify as such an abstraction. But I do not agree with your "Coders have to know" as a blanket statement.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 27.10.2016 um 19:01 schrieb Stefan Beller: Johannes Sixt writes: This is the pessimization that I am talking about. I would not mind at all if it were only for the attribute subsystem, but the proposed patch would pessimize *all* uses of pthread_mutex_lock. It would only pessimize *uninitialized* mutexes? For initialized mutexes the added burden is super cheap (one additional condition). It is not possible to mark a mutex uninitialized on Windows without an extra piece of data. A solution would become quite complicated quite quickly, and at the cost of additional operations that are in the same ballpark as an uncontended mutex. I'm not enthused. The positive aspect of this way the patch proposes would be that any future contributor not knowing the details of how to do mutexes right on Windows, would not totally break the whole system, i.e. this seems to be more maintainable in the future as it reduces the friction between pthreads mutexes and the way we can do things in Git in a platform independent way This is a non-argument. Coders have to know their tools. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Thu, Oct 27, 2016 at 10:01:02AM -0700, Stefan Beller wrote: > > That function can > > be called from main() of platforms that cannot statically initialize > > mutices, > > By main you mean the main() in common-main.c or cmd_main in git.c ? Any setup or initialization for library functions needs to go in main() of common-main.c. Otherwise, only builtins get it, and external programs are left to segfault (or whatever). > Those both look like the wrong place. Of course it would work adding it > there, but it smells like a maintenance nightmare. I agree it's ugly, but I suspect it would work OK in practice. We don't have that many mutexes and initializing them is cheap. Languages like C++ have non-constant initializers, and the compiler takes care of running the startup code before main() begins. There's no portable way to do that in C, but our cmd_main() is roughly the same thing. I still prefer the lazy initialization if it can work without incurring measurable overhead. That's the normal way to do things in C; the only complication here is the thread safety. -Peff
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
> Johannes Sixt writes:
> This is the pessimization that I am talking about. I would not mind at all if
> it were only for the attribute subsystem, but the proposed patch would
> pessimize *all* uses of pthread_mutex_lock.
It would only pessimize *uninitialized* mutexes? For initialized mutexes
the added burden is super cheap (one additional condition).
The positive aspect of this way the patch proposes would be that any
future contributor not knowing the details of how to do mutexes right
on Windows, would not totally break the whole system, i.e. this seems
to be more maintainable in the future as it reduces the friction between
pthreads mutexes and the way we can do things in Git in a platform
independent way
On Wed, Oct 26, 2016 at 11:33 PM, Junio C Hamano wrote:
>>
>> Lazy on-demand initialization as needed, perhaps? The on-demand
>> initialization mechanism may become no-op on some platforms that can
>> do static initialization.
>
> Ah, I think I misunderstood your "please rewrite". Did you mean to
> add "void attr_start(void)" helper function to attr.c that does
> series of pthread_mutex_init() calls as needed?
Well one init for now.
> That function can
> be called from main() of platforms that cannot statically initialize
> mutices,
By main you mean the main() in common-main.c or cmd_main in git.c ?
Those both look like the wrong place. Of course it would work adding it
there, but it smells like a maintenance nightmare.
And then we would modify the attr_start command depending on the
platform, i.e.
#ifdef WIN32
void attr_start(void)
{
pthread_mutex_init(..);
}
#else
void attr_start(void)
{
/* nothing as it is statically init'd */
}
#endif
> while on other platforms it can be a no-op as long as the
> variables are statically initialized? If so, that would not pessimize
> any platform, I would think.
I would think this pessimizes ALL platforms from a maintenance perspective
(but what do I know about maintaining stuff as an eager young contributor ;)
So I am willing to go that route, though I do not quite understand where exactly
you'd expect me to put the initializer as all places I can think of
are not the right
place.
Thanks,
Stefan
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Junio C Hamano writes: > Johannes Sixt writes: > >>> As many codepaths may not even need access to the attributes, I >>> doubt that would be a very productive direction to go. >> >> So, what is productive then? Pessimizing one (not exactly minor) platform? > > Lazy on-demand initialization as needed, perhaps? The on-demand > initialization mechanism may become no-op on some platforms that can > do static initialization. Ah, I think I misunderstood your "please rewrite". Did you mean to add "void attr_start(void)" helper function to attr.c that does series of pthread_mutex_init() calls as needed? That function can be called from main() of platforms that cannot statically initialize mutices, while on other platforms it can be a no-op as long as the variables are statically initialized? If so, that would not pessimize any platform, I would think.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 27.10.2016 um 08:21 schrieb Junio C Hamano: Johannes Sixt writes: As many codepaths may not even need access to the attributes, I doubt that would be a very productive direction to go. So, what is productive then? Pessimizing one (not exactly minor) platform? Lazy on-demand initialization as needed, perhaps? The on-demand initialization mechanism may become no-op on some platforms that can do static initialization. This is the pessimization that I am talking about. I would not mind at all if it were only for the attribute subsystem, but the proposed patch would pessimize *all* uses of pthread_mutex_lock. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Johannes Sixt writes: >> As many codepaths may not even need access to the attributes, I >> doubt that would be a very productive direction to go. > > So, what is productive then? Pessimizing one (not exactly minor) platform? Lazy on-demand initialization as needed, perhaps? The on-demand initialization mechanism may become no-op on some platforms that can do static initialization.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 27.10.2016 um 08:02 schrieb Junio C Hamano: Johannes Sixt writes: Am 26.10.2016 um 23:57 schrieb Stefan Beller: In Windows it is not possible to have a static initialized mutex as of now, but that seems to be painful for the upcoming refactoring of the attribute subsystem, as we have no good place to put the initialization of the attr global lock. Please rewrite the attribute system such that it can have a dynamic initialization. If you find a global initialization in main() too gross (I would agree) then setup_git_directory() might be the right place. As many codepaths may not even need access to the attributes, I doubt that would be a very productive direction to go. So, what is productive then? Pessimizing one (not exactly minor) platform? -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Johannes Sixt writes: > Am 26.10.2016 um 23:57 schrieb Stefan Beller: >> In Windows it is not possible to have a static initialized mutex as of >> now, but that seems to be painful for the upcoming refactoring of the >> attribute subsystem, as we have no good place to put the initialization >> of the attr global lock. > > Please rewrite the attribute system such that it can have a dynamic > initialization. If you find a global initialization in main() too > gross (I would agree) then setup_git_directory() might be the right > place. As many codepaths may not even need access to the attributes, I doubt that would be a very productive direction to go.
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
Am 26.10.2016 um 23:57 schrieb Stefan Beller: In Windows it is not possible to have a static initialized mutex as of now, but that seems to be painful for the upcoming refactoring of the attribute subsystem, as we have no good place to put the initialization of the attr global lock. Please rewrite the attribute system such that it can have a dynamic initialization. If you find a global initialization in main() too gross (I would agree) then setup_git_directory() might be the right place. -- Hannes
Re: [PATCH] compat: Allow static initializer for pthreads on Windows
On Wed, Oct 26, 2016 at 2:57 PM, Stefan Beller wrote: > In Windows it is not possible to have a static initialized mutex as of > now, but that seems to be painful for the upcoming refactoring of the > attribute subsystem, as we have no good place to put the initialization > of the attr global lock. > > The trick is to get a named mutex as CreateMutex[1] will return the > existing named mutex if it exists in a thread safe way, or return > a newly created mutex with that name. > > Inside the critical section of the single named mutex, we need to double > check if the mutex was already initialized because the outer check is > not sufficient. > (e.g. 2 threads enter the first condition `(!a)` at the same time, but > only one of them will acquire the named mutex first and proceeds to > initialize the given mutex a. The second thread shall not re-initialize > the given mutex `a`, which is why we have the inner condition on `(!a)`. > > Due to the use of memory barriers inside the critical section the mutex > `a` gets updated to other threads, such that any further invocation > will skip the initialization check code altogether on the first condition. > > [1] > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682411(v=vs.85).aspx > > Signed-off-by: Stefan Beller > --- > > Flying blind here, i.e. not compiled, not tested. For a system I do not > have deep knowledge of. The only help was the online documentation. This is of course a Double Check Locking Pattern, that Johannes warned about a couple of days ago. However according to https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html we can make it work by adding enough memory barriers, (See section "Making it work with explicit memory barriers").
[PATCH] compat: Allow static initializer for pthreads on Windows
In Windows it is not possible to have a static initialized mutex as of
now, but that seems to be painful for the upcoming refactoring of the
attribute subsystem, as we have no good place to put the initialization
of the attr global lock.
The trick is to get a named mutex as CreateMutex[1] will return the
existing named mutex if it exists in a thread safe way, or return
a newly created mutex with that name.
Inside the critical section of the single named mutex, we need to double
check if the mutex was already initialized because the outer check is
not sufficient.
(e.g. 2 threads enter the first condition `(!a)` at the same time, but
only one of them will acquire the named mutex first and proceeds to
initialize the given mutex a. The second thread shall not re-initialize
the given mutex `a`, which is why we have the inner condition on `(!a)`.
Due to the use of memory barriers inside the critical section the mutex
`a` gets updated to other threads, such that any further invocation
will skip the initialization check code altogether on the first condition.
[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682411(v=vs.85).aspx
Signed-off-by: Stefan Beller
---
Flying blind here, i.e. not compiled, not tested. For a system I do not
have deep knowledge of. The only help was the online documentation.
compat/win32/pthread.h | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 1c16408..a900513 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -21,9 +21,25 @@
static inline int return_0(int i) {
return 0;
}
+
+#define PTHREAD_MUTEX_INITIALIZER NULL
#define pthread_mutex_init(a,b) return_0((InitializeCriticalSection((a)), 0))
#define pthread_mutex_destroy(a) DeleteCriticalSection((a))
-#define pthread_mutex_lock EnterCriticalSection
+#define pthread_mutex_lock(a) \
+{ \
+ if (!a) { \
+ HANDLE p = CreateMutex(NULL, FALSE,
"Git-Global-Windows-Mutex"); \
+ EnterCriticalSection(p); \
+ MemoryBarrier(); \
+ if (!a)
+ pthread_mutex_init(a); \
+ MemoryBarrier(); \
+ ReleaseMutex(p); \
+ } \
+ EnterCriticalSection(a); \
+}
+
+
#define pthread_mutex_unlock LeaveCriticalSection
typedef int pthread_mutexattr_t;
--
2.10.1.508.g6572022
