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-non-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_mutexat >>>>>> 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... >> 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
