El Dissabte, 6 d'abril de 2013, a les 18:03:55, Adam Reichold va escriure: > Hello, > > Am 06.04.2013 18:00, schrieb Adam Reichold: > > Hello, > > > > Am 06.04.2013 17:45, schrieb Adam Reichold: > >> Hello again, > >> > >> Am 06.04.2013 17:40, schrieb Adam Reichold: > >>> Hello, > >>> > >>> Am 06.04.2013 17:05, schrieb Albert Astals Cid: > >>>> El Divendres, 5 d'abril de 2013, a les 17:30:49, Adam Reichold va escriure: > >>>>> Hello, > >>>>> > >>>>> Am 05.04.2013 00:38, schrieb Albert Astals Cid: > >>>>>> El Dimecres, 3 d'abril de 2013, a les 19:32:23, Albert Astals Cid va > >>>> > >>>> escriure: > >>>>>>> El Dimecres, 3 d'abril de 2013, a les 08:00:34, Thomas Freitag va > >>>> > >>>> escriure: > >>>>>>>> Am 03.04.2013 00:01, schrieb Albert Astals Cid: > >>>>>>>>> El Dimarts, 2 d'abril de 2013, a les 10:36:53, Thomas Freitag va > >>>>>> > >>>>>> escriure: > >>>>>>>>>> Am 02.04.2013 09:22, schrieb Thomas Freitag: > >>>>>>>>>>> Hi Albert! > >>>>>>>>>>> > >>>>>>>>>>> Am 27.03.2013 12:29, schrieb Thomas Freitag: > >>>>>>>>>>>> Sorry, pushed the wrong button, here my answer to the list: > >>>>>>>>>>>> > >>>>>>>>>>>> Am 27.03.2013 11:41, schrieb Albert Astals Cid: > >>>>>>>>>>>>> Why do we pass around the recursion integer around? > >>>>>>>>>>>> > >>>>>>>>>>>> No idea: The recursive integer wasn't introduced by the thread > >>>>>>>>>>>> safe > >>>>>>>>>>>> patch, it was already there. I just used it and extend some > >>>>>>>>>>>> functions > >>>>>>>>>>>> which incorrectly ignore that parameter instead of passing it > >>>>>>>>>>>> to the > >>>>>>>>>>>> called functions. Probably I missed some more than only the one > >>>>>>>>>>>> in > >>>>>>>>>>>> DCTStream::DCTStream. > >>>>>>>>>>> > >>>>>>>>>>> I just ran a regression test over my PDF files, but I didn't get > >>>>>>>>>>> any > >>>>>>>>>>> dead locks. So if You want me to solve that problem, You should > >>>>>>>>>>> provide me that PDF. (Regardless if You prefer to use recursive > >>>>>>>>>>> mutex: > >>>>>>>>>>> the endless loop detection need to pass the recursive integer > >>>>>>>>>>> everywhere!) > >>>>>>>>>> > >>>>>>>>>> Attached a patch which corrects it. I made it in my sleep, 'cause > >>>>>>>>>> the > >>>>>>>>>> dead lock was probably caused by my patch for bug 61994 (even if > >>>>>>>>>> that > >>>>>>>>>> PDF doesn't cause any dead locks). This time I also respect the > >>>>>>>>>> DCTStream usage if LIBJPEG is disabled :-) > >>>>>>>>> > >>>>>>>>> Sure, this fixes the issue (and we should commit it to help with > >>>>>>>>> the > >>>>>>>>> infinite recursion issue), but what's your opinion on making our > >>>>>>>>> mutexes > >>>>>>>>> recursive? > >>>>>>>> > >>>>>>>> I think, a quite good discussion about recursive and non recursive > >>>>>>>> mutes > >>>>>>>> was here: > >>>>>>>> http://stackoverflow.com/questions/187761/recursive-lock-mutex-vs-n > >>>>>>>> on-re > >>>>>>>> cu > >>>>>>>> rs ive-lock-mutex. This convinced me to vote for a usage of > >>>>>>>> recursive > >>>>>>>> mutex in poppler: 1. xpdf and therfore poppler wasn't really > >>>>>>>> designed > >>>>>>>> for > >>>>>>>> the usage of threads. We have it now working, but if we cherish on > >>>>>>>> the > >>>>>>>> usage of non recursive mutex we will always run into dead lock > >>>>>>>> problems > >>>>>>>> with futur patches again, so testing patches will become more > >>>>>>>> expensive. > >>>>>>>> 2. Since poppler nowhere uses emergency exits with "throw", we > >>>>>>>> don't > >>>>>>>> have the problem that we have to keep track that a thread releases > >>>>>>>> a > >>>>>>>> mutex so often it aquires it. > >>>>>>>> 3. The only (small) problem I see is the sentence "Not all systems > >>>>>>>> supporting pthreads also support recursive mutexes, but if they > >>>>>>>> want to > >>>>>>>> be POSIX conform, they have to > >>>>>>>> <http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mu > >>>>>>>> texat > >>>>>>>> tr > >>>>>>>> _g ettype.html>." > >>>>>>> > >>>>>>> To be honest i don't think we should care much about the minority of > >>>>>>> systems there that don't support recursive mutexes. > >>>>>> > >>>>>> Here's a quick patch I've hacked together (still needs some work in > >>>>>> the > >>>>>> buildsystem side). > >>>>> > >>>>> Not something that breaks either, but maybe a performance improvement: > >>>>> According to the manual page of 'phtread_mutexattr_init', a single > >>>>> mutex > >>>>> attribute object can be used to initialize several mutexes, hence we > >>>>> possibly don't need to keep it in the GooLock structure and could use > >>>>> just a single instance that is initialized once. Not sure whether this > >>>>> really hurts with the number of mutexes we have. > >>>> > >>>> I tried doing that but then i needed a "global" mutex attribute that i > >>>> needed to initialize, and that meant i needed to include a mutex to > >>>> protect the initialization of the attribute or "force" all the mutexes > >>>> to be created in the same thread. At the end i went the "easy" way of > >>>> having one mutex attribute per mutex, it's not like we are going to > >>>> use that much more memory and it's just easier to understand codewise. > >>> > >>> I see and I agree that it is probably simpler that way. But then I think > >>> there should be a call to 'pthread_mutexattr_destroy' in > >>> 'gDestroyMutex', shouldn't there? > >> > >> Sorry, just noticed that you already included it in the commit... > > > > Again I am sorry (must be the weekend), but after reading the POSIX > > programmer's manual page again, I think the 'mutexattr' object does not > > need to survive the mutex itself, e.g. > > > > typedef pthread_mutex_t GooMutex; > > > > inline void gInitMutex(GooMutex *m) { > > > > pthread_mutexattr_t mutexattr; > > pthread_mutexattr_init(&mutexattr); > > pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_RECURSIVE); > > pthread_mutex_init(&m, &mutexattr); > > > > } > > Of course, include the destruction call, i.e. > > inline void gInitMutex(GooMutex *m) { > pthread_mutexattr_t mutexattr; > pthread_mutexattr_init(&mutexattr); > pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_RECURSIVE); > pthread_mutex_init(&m, &mutexattr); > pthread_mutexattr_destroy(&mutexattr); > }
Makes sense. Pushed. Cheers, Albert > > > should work as well. > > > > Best regards, Adam. > > > > P.S.: At least this is how I would interpret > > > > After a mutex attributes object has been used to initialize one or more > > mutexes, any function affecting the attributes object (including > > destruction) shall not affect any previously initialized mutexes. > > > > from the manual page. > > > >>>> Cheers, > >>>> > >>>> Albert > >>>> > >>>> P.S: I had to add some -lpthread to the cmake buildsystem but not to > >>>> the > >>>> automake one, find it a bit weird to be honest but it's what i needed > >>>> to make it compile in both. > >>> > >>> Yes, sounds strange. Maybe the 'ACX_PTHREAD' macro has some side effects > >>> w.r.t. that? > >>> > >>> Best regards, Adam. > >>> > >>>>> Best regards, Adam. > >>>>> > >>>>>> I'll finish it properly tomorrow but meanwhile can you guys check if > >>>>>> it > >>>>>> breaks something related to the multithreading side? > >>>>>> > >>>>>> I haven't touched the windows side since somewhere i've read that > >>>>>> CriticalSections are already recursive. Can anyone confirm? > >>>>>> > >>>>>> Cheers, > >>>>>> > >>>>>> Albert > >>>>>>> > >>>>>>> Cheers, > >>>>>>> > >>>>>>> Albert > >>>>>>>> > >>>>>>>> Cheers, > >>>>>>>> Thomas > >>>>>>>> > >>>>>>>>> It should help us making the code simpler since we could just drop > >>>>>>>>> all > >>>>>>>>> the > >>>>>>>>> DoNotLockMutex cases, no? > >>>>>>>>> > >>>>>>>>> Cheers, > >>>>>>>>> > >>>>>>>>> Albert > >>>>>>>>>> > >>>>>>>>>> Cheers, > >>>>>>>>>> Thomas > >>>>>>>>>> > >>>>>>>>>>> Cheers, > >>>>>>>>>>> Thomas > >>>>>>>>>>> > >>>>>>>>>>>>> Wouldn't a real recursive mutex be enough? > >>>>>>>>>>>> > >>>>>>>>>>>> No idea: I used in the thread safe patch just the pthread_lock > >>>>>>>>>>>> which > >>>>>>>>>>>> was defined. I'm not knowing what happens when we change it > >>>>>>>>>>>> globally > >>>>>>>>>>>> i.e. if it affects other usages of the GooMutex. Just feel free > >>>>>>>>>>>> to > >>>>>>>>>>>> change it, You should have the better overview of usages than I > >>>>>>>>>>>> have. > >>>>>>>>>>>> > >>>>>>>>>>>> Cheers, > >>>>>>>>>>>> Thomas > >>>>>>>>>>>> > >>>>>>>>>>>>> I'm asking because i have a document here (that sadly i can't > >>>>>>>>>>>>> share) > >>>>>>>>>>>>> that is deadlocking > >>>>>>>>>>>>> itself there because we are not passing the recursion integer > >>>>>>>>>>>>> everywhere (we lose it in DCTStream::DCTStream) > >>>>>>>>>>>>> as shown in this backtrace > >>>>>>>>>>>>> > >>>>>>>>>>>>> #0 __lll_lock_wait () at > >>>>>>>>>>>>> ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135 > >>>>>>>>>>>>> #1 0x00007ffff599217c in _L_lock_982 () from > >>>>>>>>>>>>> /lib/x86_64-linux-gnu/libpthread.so.0 > >>>>>>>>>>>>> #2 0x00007ffff5991fcb in __GI___pthread_mutex_lock > >>>>>>>>>>>>> (mutex=0x71dc10) > >>>>>>>>>>>>> at pthread_mutex_lock.c:64 > >>>>>>>>>>>>> #3 0x00007ffff5f02f73 in MutexLocker::MutexLocker > >>>>>>>>>>>>> (this=0x7fffffffcfb0, mutexA=0x71dc10, modeA=DoLockMutex) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/goo/GooMutex.h:72 > >>>>>>>>>>>>> #4 0x00007ffff5fac8e2 in XRef::fetch (this=0x71db50, num=5, > >>>>>>>>>>>>> gen=0, > >>>>>>>>>>>>> obj=0x7fffffffd0b0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1137 > >>>>>>>>>>>>> #5 0x00007ffff5f8a013 in Object::fetch (this=0x71fe08, > >>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd0b0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122 > >>>>>>>>>>>>> #6 0x00007ffff5f19de4 in Dict::lookup (this=0x71fb60, > >>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) > >>>>>>>>>>>>> at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256 > >>>>>>>>>>>>> #7 0x00007ffff5f032e9 in Object::dictLookup > >>>>>>>>>>>>> (this=0x7fffffffd5a0, > >>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) > >>>>>>>>>>>>> at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315 > >>>>>>>>>>>>> #8 0x00007ffff601f8a3 in DCTStream::DCTStream (this=0x7832b0, > >>>>>>>>>>>>> strA=0x720780, colorXformA=-1, dict=0x7fffffffd5a0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/DCTStream.cc:72 > >>>>>>>>>>>>> #9 0x00007ffff5f9bf6d in Stream::makeFilter (this=0x720780, > >>>>>>>>>>>>> name=0x71fef0 "DCTDecode", str=0x720780, > >>>>>>>>>>>>> params=0x7fffffffd1e0, > >>>>>>>>>>>>> recursion=1, dict=0x7fffffffd5a0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:311 > >>>>>>>>>>>>> #10 0x00007ffff5f9b71a in Stream::addFilters (this=0x720780, > >>>>>>>>>>>>> dict=0x7fffffffd5a0, recursion=1) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:184 > >>>>>>>>>>>>> #11 0x00007ffff5f9204b in Parser::makeStream (this=0x71fcd0, > >>>>>>>>>>>>> dict=0x7fffffffd5a0, fileKey=0x0, encAlgorithm=cryptRC4, > >>>>>>>>>>>>> keyLength=1146103040, objNum=3, objGen=0, recursion=1, > >>>>>>>>>>>>> strict=false) > >>>>>>>>>>>>> at /home/tsdgeos/devel/poppler/poppler/Parser.cc:280 > >>>>>>>>>>>>> #12 0x00007ffff5f918e6 in Parser::getObj (this=0x71fcd0, > >>>>>>>>>>>>> obj=0x7fffffffd5a0, simpleOnly=false, fileKey=0x0, > >>>>>>>>>>>>> encAlgorithm=cryptRC4, keyLength=1146103040, objNum=3, > >>>>>>>>>>>>> objGen=0, > >>>>>>>>>>>>> recursion=0, strict=false) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Parser.cc:131 > >>>>>>>>>>>>> #13 0x00007ffff5facdd3 in XRef::fetch (this=0x71db50, num=3, > >>>>>>>>>>>>> gen=0, > >>>>>>>>>>>>> obj=0x7fffffffd5a0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1197 > >>>>>>>>>>>>> #14 0x00007ffff5f8a013 in Object::fetch (this=0x71e128, > >>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd5a0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122 > >>>>>>>>>>>>> #15 0x00007ffff5f19de4 in Dict::lookup (this=0x71e660, > >>>>>>>>>>>>> key=0x7206e0 > >>>>>>>>>>>>> "Im1", obj=0x7fffffffd5a0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256 > >>>>>>>>>>>>> #16 0x00007ffff5f032e9 in Object::dictLookup (this=0x71e598, > >>>>>>>>>>>>> key=0x7206e0 "Im1", obj=0x7fffffffd5a0, recursion=0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315 > >>>>>>>>>>>>> #17 0x00007ffff5f2a647 in GfxResources::lookupXObject > >>>>>>>>>>>>> (this=0x71e590, name=0x7206e0 "Im1", obj=0x7fffffffd5a0) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:411 > >>>>>>>>>>>>> #18 0x00007ffff5f3f41e in Gfx::opXObject (this=0x71e440, > >>>>>>>>>>>>> args=0x7fffffffd720, numArgs=1) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:4114 > >>>>>>>>>>>>> #19 0x00007ffff5f2bfd6 in Gfx::execOp (this=0x71e440, > >>>>>>>>>>>>> cmd=0x7fffffffd6e0, args=0x7fffffffd720, numArgs=1) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:858 > >>>>>>>>>>>>> #20 0x00007ffff5f2b82b in Gfx::go (this=0x71e440, > >>>>>>>>>>>>> topLevel=true) at > >>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:717 > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Cheers, > >>>>>>>>>>>>> > >>>>>>>>>>>>> Albert > >>>>>>>>>>>>> > >>>>>>>>>>>>> _______________________________________________ > >>>>>>>>>>>>> poppler mailing list > >>>>>>>>>>>>> [email protected] > >>>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>>>>>>>> > >>>>>>>>>>>>> . > >>>>>>>>>>>> > >>>>>>>>>>>> _______________________________________________ > >>>>>>>>>>>> poppler mailing list > >>>>>>>>>>>> [email protected] > >>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>>>>>>> > >>>>>>>>>>>> . > >>>>>>>>>>> > >>>>>>>>>>> _______________________________________________ > >>>>>>>>>>> poppler mailing list > >>>>>>>>>>> [email protected] > >>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>>>>>> > >>>>>>>>>>> . > >>>>>>>>> > >>>>>>>>> _______________________________________________ > >>>>>>>>> poppler mailing list > >>>>>>>>> [email protected] > >>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>>>> > >>>>>>>>> . > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> poppler mailing list > >>>>>>> [email protected] > >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>> > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> poppler mailing list > >>>>>>> [email protected] > >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>> > >>>>> _______________________________________________ > >>>>> poppler mailing list > >>>>> [email protected] > >>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>> > >>>> _______________________________________________ > >>>> poppler mailing list > >>>> [email protected] > >>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>> > >>> _______________________________________________ > >>> poppler mailing list > >>> [email protected] > >>> http://lists.freedesktop.org/mailman/listinfo/poppler > >> > >> _______________________________________________ > >> poppler mailing list > >> [email protected] > >> http://lists.freedesktop.org/mailman/listinfo/poppler > > > > _______________________________________________ > > poppler mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ > poppler mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/poppler _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
