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

Wed Apr 10 07:56:04 PDT 2013

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. :-)

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
> 