[poppler] XRef::fetch mutex lock

Adam Reichold adamreichold at myopera.com
Sat Apr 6 08:45:38 PDT 2013 

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
>>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>>
>>>>>>>>>>> .
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> poppler mailing list
>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>
>>>>>>>>>> .
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> poppler mailing list
>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>
>>>>>>>>> .
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> poppler mailing list
>>>>>>> poppler at lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>
>>>>>>> .
>>>>>
>>>>> _______________________________________________
>>>>> poppler mailing list
>>>>> poppler at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> poppler mailing list
>>>>> poppler at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>
>>> _______________________________________________
>>> poppler mailing list
>>> poppler at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>> _______________________________________________
>> poppler mailing list
>> poppler at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>

More information about the poppler mailing list