jkriege2 / JKQtPlotter

an extensive Qt5 & Qt6 Plotter framework (including a feature-richt plotter widget, a speed-optimized, but limited variant and a LaTeX equation renderer!), written fully in C/C++ and without external dependencies
http://jkriege2.github.io/JKQtPlotter/index.html
GNU Lesser General Public License v2.1
889 stars 190 forks source link

Parallelize images creations #95

Closed sim186 closed 10 months ago

sim186 commented 1 year ago

Hi,

I have the need to produce multiple plots and save them as images. I'm implementing some performance improvements. My first thought is to, since I have all the necessary data in a vector of structures, to make the for loop parallel using OpenMP instantiating a new plotter (then indipendent data stores) in every cycle in this way I could make them independently. The first tries shows some race condition between threads. I think the thread are indipendent so my question is: in the plotter implementation there is some kind of optimization in order to not instanciate multiple time the plotter but utilizing the same memory underneath?

Thanks

jkriege2 commented 1 year ago

HI!

I had a look through the code ... and yes, there are several (actually quite a lot) static variables and caches that are used to speed up the plotter (and JKQTMathText). All of these are not secured/serialized by a Mutex, so they can (and will) lead to race conditions ... at the moment it seems like a lot of work to add such safe-guards. Also these could then hamper with the speed of such parallel plotting, as they would lead to an intrinsic serialization and add some unneeded extra slowdown (acquiring/releasing mutexes) when only one thread is used.

As a side-node: Using static variables here was a conscious decision, so we can obtain a certain speedup in applications that use multiple JKQTPlotter instances in one GUI.

Sorry, I don't have better news on this :-(

Best, JAN

PS: If you come up with a good solution to the points mentioned above, I would be happy to discuss it and maybe also merge a PR!

jkriege2 commented 1 year ago

I looked a bit into this ... I may change the implementation, so it is thread-safe, but that will be a lot of work ... I started here: https://github.com/jkriege2/JKQtPlotter/commit/cf43dc4a7ef5822026a5de569ba627eeeb722ca1 (so you see what needs to be done)

... any help is welcome ... I will keep these things in mind, but it will take long until everything is changed!

jkriege2 commented 1 year ago

Hi @sim186 ,

yesterday and today, I tried to protect all the static variables that are used by JKQTBasePlotter and JKQTMathText ... at least I hope I did so ... I hope now it is possible to use at least JKQTBasePlotter for parallel plot creation!

Best, JAN

jkriege2 commented 1 year ago

BTW: Do you have a test program for that use case?

jkriege2 commented 11 months ago

@sim186 Did you check, whether parallel plotting works now?

Best, JAN

sim186 commented 10 months ago

Hi,

I totally forgot about this. I was just now doing some tests and I had problems so call some methods within the threads like:

- setFont()
- zoomToFit()
- saveAsPixelImage()
- clearGraphs()

all those I guess should be moved to the gui thread, so I started to call them into the invokeMethod, but I think in the end the advantage of parallelising the images creation in this case will be counterweigthed by the decision to move those calls to the main thread. I found that using grabPixelImage along with the turbojpeg library to save the image provide me the performance is am looking for.

I don't know what are your thoughts about this.

jkriege2 commented 10 months ago

Mmmhhh ... It tested it myself (see https://github.com/jkriege2/JKQtPlotter/commit/aa2fcb108d0018139b59811d70d6df39b71a20ad) and JKQTBasePlotter seems to be re-entrant now (see the new example https://github.com/jkriege2/JKQtPlotter/tree/master/examples/multithreaded .

The class JKQTPlotter cannot be used in parallel threads, as it is a GUI class!

The speedup of parallelization is not great (possibly due to the synchronizations of the internal caches that I introduced, but there ist room for optimization.

Does that help you in any way?

jkriege2 commented 10 months ago

I made some more modifications, which should improve the speedup further.

jkriege2 commented 10 months ago

I think I would close this, if you don't have anything else on this topic?

sim186 commented 10 months ago

I think is complete I will need just to adjust the code based on your suggestions. Thanks for the explaination. 👍