[PATCH] use pthreads for cache on cygwin
Hello, the "native" thread API for cygwin is pthreads, the threading API we currently use for MinGW is not available for it (my fault for not checking), and the one we used before to the best of my knowledge would have memleaks on cygwin, too. Thus I propose attached patch which uses pthread_create etc. It depends on the PTHREAD_CACHE define so it will be easy to activate on other systems, too (I tested it on Linux, someone on -users tested it for cygwin), but for now will only be used on Cygwin. Any objections to it? Greetings, Reimar Döffinger
On Fri, Nov 14, 2008 at 08:27:55PM +0100, Reimar Döffinger wrote:
the "native" thread API for cygwin is pthreads, the threading API we currently use for MinGW is not available for it (my fault for not checking), and the one we used before to the best of my knowledge would have memleaks on cygwin, too. Thus I propose attached patch which uses pthread_create etc. It depends on the PTHREAD_CACHE define so it will be easy to activate on other systems, too (I tested it on Linux, someone on -users tested it for cygwin), but for now will only be used on Cygwin. Any objections to it?
Sorry if you consider it hasty, but after another cygwin user complaining I decided to apply it, thinking it can not really make anything worse. If you have complaints about it I'll try to fix it soonish (e.g. I think there probably should be some configure logic for all this, but since it is currently such an OS-specific mess I am unsure how to do it best). Greetings, Reimar Döffinger
On Fri, Nov 14, 2008 at 08:27:55PM +0100, Reimar Döffinger wrote:
the "native" thread API for cygwin is pthreads, the threading API we currently use for MinGW is not available for it (my fault for not checking), and the one we used before to the best of my knowledge would have memleaks on cygwin, too. Thus I propose attached patch which uses pthread_create etc. It depends on the PTHREAD_CACHE define so it will be easy to activate on other systems, too (I tested it on Linux, someone on -users tested it for cygwin), but for now will only be used on Cygwin. Any objections to it?
--- stream/cache2.c (revision 27905) +++ stream/cache2.c (working copy) @@ -16,15 +16,22 @@
+#ifdef __CYGWIN__ +#define PTHREAD_CACHE 1 +#endif
This should be in configure. Diego
On Sat, Nov 15, 2008 at 09:34:56PM +0100, Diego Biurrun wrote:
On Fri, Nov 14, 2008 at 08:27:55PM +0100, Reimar Döffinger wrote:
the "native" thread API for cygwin is pthreads, the threading API we currently use for MinGW is not available for it (my fault for not checking), and the one we used before to the best of my knowledge would have memleaks on cygwin, too. Thus I propose attached patch which uses pthread_create etc. It depends on the PTHREAD_CACHE define so it will be easy to activate on other systems, too (I tested it on Linux, someone on -users tested it for cygwin), but for now will only be used on Cygwin. Any objections to it?
--- stream/cache2.c (revision 27905) +++ stream/cache2.c (working copy) @@ -16,15 +16,22 @@
+#ifdef __CYGWIN__ +#define PTHREAD_CACHE 1 +#endif
This should be in configure.
I know, I just was not sure about the best logic. I still don't have Cygwin to test myself, but attached patch would probably work. I don't like it much though because it leaves the cache checks a bit spread all over the place.
On Sat, Nov 15, 2008 at 11:24:35PM +0100, Reimar Döffinger wrote:
On Sat, Nov 15, 2008 at 09:34:56PM +0100, Diego Biurrun wrote:
On Fri, Nov 14, 2008 at 08:27:55PM +0100, Reimar Döffinger wrote:
the "native" thread API for cygwin is pthreads, the threading API we currently use for MinGW is not available for it (my fault for not checking), and the one we used before to the best of my knowledge would have memleaks on cygwin, too. Thus I propose attached patch which uses pthread_create etc. It depends on the PTHREAD_CACHE define so it will be easy to activate on other systems, too (I tested it on Linux, someone on -users tested it for cygwin), but for now will only be used on Cygwin. Any objections to it?
--- stream/cache2.c (revision 27905) +++ stream/cache2.c (working copy) @@ -16,15 +16,22 @@
+#ifdef __CYGWIN__ +#define PTHREAD_CACHE 1 +#endif
This should be in configure.
I know, I just was not sure about the best logic. I still don't have Cygwin to test myself, but attached patch would probably work. I don't like it much though because it leaves the cache checks a bit spread all over the place.
It's better than what we have now, so commit it. Diego
On Fri, Nov 28, 2008 at 05:04:03PM +0100, Diego Biurrun wrote:
On Sat, Nov 15, 2008 at 11:24:35PM +0100, Reimar Döffinger wrote:
I know, I just was not sure about the best logic. I still don't have Cygwin to test myself, but attached patch would probably work. I don't like it much though because it leaves the cache checks a bit spread all over the place.
It's better than what we have now, so commit it.
Applied then. Maybe someone makes it a proper configure option or something, there may be a few people who might like to try it on Linux or some other OSes, it might work better with gmplayer (it might also crash more, depending whether GTK is thread-safe enough). Greetings, Reimar Döffinger
I know, I just was not sure about the best logic. I still don't have Cygwin to test myself, but attached patch would probably work. I don't like it much though because it leaves the cache checks a bit spread all over the place.
It's better than what we have now, so commit it.
Applied then. Maybe someone makes it a proper configure option or something, there may be a few people who might like to try it on Linux or some other OSes, it might work better with gmplayer (it might also crash more, depending whether GTK is thread-safe enough).
It'd be nice to have on Linux and Darwin. I had my own patch to do something similar... I was going develop it a bit further, clean it up then sumbit it but you beat me to the punch :) The only _major_ difference I had was I moved the main cache loop into a separate function instead of #ifdef'ing the closing brace of stream_enable_cache (looks kinda sloppy). I haven't tested this version yet, but I will get around to it today or tomorrow... There's one problem I can see, pthread_t may or may not be a "unsigned int" sized value (esp. 64 bit systems) so I propose the following patch to stream/cache2.h. -DrD-
On Mon, Dec 01, 2008 at 02:45:33PM -0800, David DeHaven wrote:
There's one problem I can see, pthread_t may or may not be a "unsigned int" sized value (esp. 64 bit systems) so I propose the following patch to stream/cache2.h.
It is never used for pthread. It's always a good idea to svn up before making patches.
It is never used for pthread. It's always a good idea to svn up before making patches.
No, I just made a bad assumption. I used it to make sure the thread terminated properly in my implementation. -DrD-
Hello, On Mon, Dec 01, 2008 at 03:17:39PM -0800, David DeHaven wrote:
It is never used for pthread. It's always a good idea to svn up before making patches.
No, I just made a bad assumption. I used it to make sure the thread terminated properly in my implementation.
I know, I considered doing something like that. But the thing is, at least on Windows there is (AFAICT) no way to forcibly terminate a thread without leaking memory, and even everywhere else it is a bad idea. Greetings, Reimar Döffinger
It is never used for pthread. It's always a good idea to svn up before making patches.
No, I just made a bad assumption. I used it to make sure the thread terminated properly in my implementation.
I know, I considered doing something like that. But the thing is, at least on Windows there is (AFAICT) no way to forcibly terminate a thread without leaking memory, and even everywhere else it is a bad idea.
Ah, no, I didn't force it to terminate, I've never had luck with that myself (though I have used pthread_join in the past). I've had so many issues with deadlocking and race conditions in multithreaded apps that I tend to spin the main thread until the thread terminates (using flags) to make sure it's dead, but it terminates itself. I did that here because there was some condition that would cause a deadlock when trying to terminate the app, I don't know if that's still the case or not. We'll find out as soon as it's done building. -DrD-
Currently _def_pthread_cache never gets emitted in config.h (patch below). Otherwise it seems to be working fine on darwin/Mac OS X. No cache thread related crashes or deadlocks yet. It _seems_ sane enough to enable if pthreads are enabled (just remove the outer if/fi block in configure), but I'll let you guys make the call on that one. I just changed to "if cygwin || darwin" for testing here. -DrD-
On Tue, Dec 02, 2008 at 11:55:45AM -0800, David DeHaven wrote:
Currently _def_pthread_cache never gets emitted in config.h (patch below).
Applied by Reimar. Diego
participants (3)
-
David DeHaven -
Diego Biurrun -
Reimar Döffinger