[poppler] RFC: upstream optional threading support in pdftoppm for simple testing

Adam Reichold adamreichold at myopera.com
Mon Jun 3 08:37:16 PDT 2013 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Albert,

Am 02.06.2013 23:30, schrieb Albert Astals Cid:
> El Diumenge, 2 de juny de 2013, a les 12:10:19, Adam Reichold va
> escriure:
>> Hello Albert,
>> 
>> The test case was part of bug #59927 "make qt4 frontend
>> thread-safe" [1]. I took that code and added a wee bit of
>> configurability and a lock that should serialize the parts
>> depending on annotation life cycle.
> 
> Pushed, gave the file a "better" (imho) name.

I agree, the original name was somewhat silly...

Sadly enough, I made (at least) one small mistake concerning the
command-line arguments, c.f. the attached patch.

Best regards, Adam.

> Cheers, Albert
> 
>> 
>> Best regards, Adam.
>> 
>> P.S.: I could also rework this into a single source file if you
>> prefer that.
>> 
>> [1] https://bugs.freedesktop.org/show_bug.cgi?id=59927
>> 
>> Am 01.06.2013 19:58, schrieb Albert Astals Cid:
>>> El Dimecres, 10 d'abril de 2013, a les 16:56:04, Adam Reichold
>>> va
> escriure:
>>>> Hell Albert,
>>>> 
>>>> Am 09.04.2013 00:51, schrieb Albert Astals Cid:
>>>>> El Dilluns, 8 d'abril de 2013, a les 06:53:10, Thomas
>>>>> Freitag va
> escriure:
>>>>>> Am 08.04.2013 00:42, schrieb Albert Astals Cid:
>>>>>>> El Diumenge, 7 d'abril de 2013, a les 22:07:46, Thomas
>>>>>>> Freitag va
>>>>> 
>>>>> escriure:
>>>>>>>> Am 07.04.2013 21:12, schrieb Albert Astals Cid:
>>>>>>>>> El Diumenge, 7 d'abril de 2013, a les 16:31:52,
>>>>>>>>> Adam Reichold va
>>>>> 
>>>>> escriure:
>>>>>>>>>> Hello,
>>>>>>>>>> 
>>>>>>>>>> Am 07.04.2013 16:13, schrieb Albert Astals Cid:
>>>>>>>>>>> El Dissabte, 6 d'abril de 2013, a les 17:43:54,
>>>>>>>>>>> Adam Reichold va
>>>>>>> 
>>>>>>> escriure:
>>>>>>>>>>>> Hello,
>>>>>>>>>>>> 
>>>>>>>>>>>> Am 06.04.2013 17:14, schrieb Albert Astals
>>>>>>>>>>>> Cid:
>>>>>>>>>>>>> El Divendres, 5 d'abril de 2013, a les
>>>>>>>>>>>>> 21:43:28, Adam Reichold va
>>>>>>>>>>> 
>>>>>>>>>>> escriure:
>>>>>>>>>>>>>> Hello again,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> I was a bit in a rush at the first try.
>>>>>>>>>>>>>> Sorry for that, I tidied it up slightly.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Maybe we should rename from
>>>>>>>>>>>>> UTILS_USE_THREAD to UTILS_USE_PTHREAD ?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Or add a comment somewhere that we only
>>>>>>>>>>>>> support pthreads for now somewhere?
>>>>>>>>>>>> 
>>>>>>>>>>>> I would be fine with both.
>>>>>>>>>>>> 
>>>>>>>>>>>> Actually, since this is mostly meant for
>>>>>>>>>>>> testing, I would be fine with not making it
>>>>>>>>>>>> accessible via autotools or CMake at all,
>>>>>>>>>>>> i.e. just add the definition to 'config.h'
>>>>>>>>>>>> manually when and if we need it.
>>>>>>>>>>> 
>>>>>>>>>>> Makes sense to me, code-wise what's the
>>>>>>>>>>> difference between this and the code Thomas
>>>>>>>>>>> posted in the threading bug? Do you think this
>>>>>>>>>>> is simpler/easier to understand?
>>>>>>>>>> 
>>>>>>>>>> Yes, the difference is that I left out the
>>>>>>>>>> Windows-specific part and tried to keep it as
>>>>>>>>>> simple as possible. For example, I think 
>>>>>>>>>> synchronizing on the job queue is simpler than
>>>>>>>>>> synchronizing on the thread state. But of course,
>>>>>>>>>> my implementation is not very efficient in terms
>>>>>>>>>> of performance, just sufficient for testing.
>>>>>>>>> 
>>>>>>>>> Thomas would you be OK if we merge this patchset or
>>>>>>>>> you'd prefer yours (more complex?) to be in?
>>>>>>>> 
>>>>>>>> Oh, I never thought to get a question like this. To
>>>>>>>> answer it, I need to to go a little bit more far
>>>>>>>> afield: When I started to implement it, first of all
>>>>>>>> I need a test case. Therefore I made that hack to 
>>>>>>>> pdftoppm to use multiple threads under Windows (still
>>>>>>>> my favorite programming platform, I'm too old to
>>>>>>>> change my habits), the pthread version I made much
>>>>>>>> later to run it over the PDF suite and so that You
>>>>>>>> can test it, too. But it was never made to publish
>>>>>>>> it. I think that a multi threading version of
>>>>>>>> pdftoppm is not really necessary for the community.
>>>>>>> 
>>>>>>> If we replace community by final user, agreed.
>>>>>>> 
>>>>>>>> BUT: because we now support multi threading, we need
>>>>>>>> a test case in general. I haven't review Adam's patch
>>>>>>>> in detail, but if You think it is sufficient as test
>>>>>>>> case (and I don't think, that anyone need a Windows 
>>>>>>>> test case), I can live with it. If You otherwise mean
>>>>>>>> that we need a more general support also on Windows,
>>>>>>>> I'll spend one or two weekends to revise my
>>>>>>>> solution.
>>>>>>> 
>>>>>>> Honestly *I* don't need Windows support, but that's not
>>>>>>> *my* project but a community one and you seem to need
>>>>>>> it so we need it.
>>>>>>> 
>>>>>>> What I didn't originally like about your patch is that
>>>>>>> it was a bit too much intrusive for my taste, and to be
>>>>>>> honest Adam's a bit too.
>>>>>> 
>>>>>> The advantage of a "pdftoppm" test case is that we can
>>>>>> use it in the regression test without any manipulation
>>>>>> and can compare the multi threading results with the
>>>>>> "normal" results. And with the actual state we still have
>>>>>> different results 1. with damaged PDF where the "normal"
>>>>>> pdftoppm produces output for pages which can not be
>>>>>> rendered by copying the last producable page. 2. with the
>>>>>> type 3 fonts where the caching produces "random" result. 
>>>>>> 3. with non embedded fonts where the decision which
>>>>>> external font should be used is sometimes different. But
>>>>>> to solve this even I don't need a windows solution. I did
>>>>>> need it only in the past to set breakpoints to have an
>>>>>> easier look at variables, memory and call stack.
>>>>>> 
>>>>>>> So that got me thinking into the fact that we don't
>>>>>>> really need much of the pdftoppm code, we just need to
>>>>>>> create a SplashOutputDev and feed it into a few
>>>>>>> threads.
>>>>>>> 
>>>>>>> So what do you think of "separate" non pdftoppm code in
>>>>>>> the test/ folder?
>>>>>>> 
>>>>>>> This way we can even have a thread_pthread_test.cpp and
>>>>>>> a thread_windows_test.cpp, etc. without "polluting" the
>>>>>>> regular pdftoppm code.
>>>>>> 
>>>>>> What do You think about Adam's qt testcase? It has the
>>>>>> advantage of not only test the rendering, even if it need
>>>>>> some enhancements, because You still cannot modify the
>>>>>> same annotation from different threads at the same time.
>>>>> 
>>>>> I do like it, and being Qt has the benefit of being
>>>>> multiplatform already, but it has the problem of not being
>>>>> pdftoppm and thus not a drop-in replacement.
>>>>> 
>>>>> Oh the decisions one has to take :D
>>>>> 
>>>>> Ok, so what does this like?
>>>>> 
>>>>> Take Adam's "qt test" and add it to qt/tests to have a
>>>>> "multiplatform stressing tool"
>>>>> 
>>>>> Take Adam's "non introsuive pthread additions to pdftoppm"
>>>>> to have a "drop in replacement for the test suite"
>>>>> 
>>>>> This way we get both sides happy?
>>>>> 
>>>>> Yes?
>>>> 
>>>> Sorry for being so late to answer, I was occupied at work.
>>>> And yes, this would make me happy. :-)
>>> 
>>> Adam I've commited the pdftoppm-pthread code but i can't find
>>> the qt4 code, it was in some bug right? Do you remember where?
>>> 
>>> Cheers,
>>> 
>>> Albert
>>>> 
>>>> Best regards, Adam.
>>>> 
>>>>> Albert
>>>>> 
>>>>>> Cheers, Thomas
>>>>>> 
>>>>>>> Thoughts?
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> 
>>>>>>> Albert
>>>>>>>> 
>>>>>>>> So I band the ball back to You. Cheers, Thomas
>>>>>>>> 
>>>>>>>>> Cheers,
>>>>>>>>> 
>>>>>>>>> Albert
>>>>>>>>>> 
>>>>>>>>>> Best regards, Adam.
>>>>>>>>>> 
>>>>>>>>>>> Cheers,
>>>>>>>>>>> 
>>>>>>>>>>> Albert
>>>>>>>>>>>> 
>>>>>>>>>>>> Best regards, Adam.
>>>>>>>>>>>> 
>>>>>>>>>>>>> Besides that it looks ok-ish in a quick
>>>>>>>>>>>>> look.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Anyone has a comment?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Albert
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Best regards, Adam.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Am 05.04.2013 19:27, schrieb Adam
>>>>>>>>>>>>>> Reichold:
>>>>>>>>>>>>>>> Hello everyone,
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> To make it easier for us to test
>>>>>>>>>>>>>>> changes w.r.t. to threading, I would 
>>>>>>>>>>>>>>> propose to commit a simple
>>>>>>>>>>>>>>> implementation of threading in 
>>>>>>>>>>>>>>> 'pdftoppm' to master.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> The attached patch contains a very
>>>>>>>>>>>>>>> simple implementation that is not 
>>>>>>>>>>>>>>> focused on maximal performance but
>>>>>>>>>>>>>>> should suffice to stress the locking 
>>>>>>>>>>>>>>> inside Poppler's core. I opted to
>>>>>>>>>>>>>>> implement only the POSIX approach since
>>>>>>>>>>>>>>> I suppose POSIX systems are where most
>>>>>>>>>>>>>>> of us test and the code is hopefully
>>>>>>>>>>>>>>> simple and short enough not become a
>>>>>>>>>>>>>>> maintenance burden.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> What do you think?
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Best regards, Adam.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 
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
>>>>> 
>>>>> _______________________________________________ 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
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)

iQEcBAEBAgAGBQJRrLgsAAoJEPSSjE3STU34CuwH/iAiTHpkN2YmIMIiYUyHsSlA
8KF8SSOcB3TN0QJY5nbRW9stWfSoQGwWP+l79LyTTNe1h5WKRRB4H701XhZM+r8t
I+cNSiiyItPntoQJ0srebYUujTnGPQkb+Nu58ineCDSnvkKXPoYE6g3NSYk3uWe5
3qST+W3Q7Fa7DikGLt+Uca0scYWuPJCDWiDKR1RlW3nPV+rZ8i4f5iGw65Q5L0Sh
DNkMNiA4kWb/mC6mREIl4gyH+xZ498z1yTl9S0gb0VGWcD7kdOE2u55jVysk3QjB
ASefr8QRBhOFUl+yLIIW09+I0iPuWY27LIyqvlijTMnzhzDCCxrnooHbpugxY8s=
=f6Vf
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-argc.diff
Type: text/x-patch
Size: 412 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20130603/66d1e9fb/attachment.bin>

More information about the poppler mailing list