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. 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 > >>>>>>>>>>>>> [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 > >>> > >>> _______________________________________________ > >>> 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
