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-recu >>> 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_mutexattr >>> _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. 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
