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? > 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
