Please check my coding for Thread Safety issues

[louwin]

I struggled through writing a Mandelbrot program. Over time I got it right where I wanted it. It works almost perfectly.

Then I thought I could speed it up some with 6 threads, maybe 8 later?

I changed the the method to output the Mandelbrot in 6 horizontal "slices' of 180 scan lines each. A perfectly working program (1920*1080) got completely garbled :(

If I run the method (Mandelbrot) as a single thread (all 1080 scan lines) it works perfectly. If I code 2 or more threads - no good. All my research suggests all "written" variables are local and "read" variables can be global!?

Like I said, if I run ONE thread or 6 (unthreaded) method calls it works perfectly.
Could someone look at the code (snipet) and point out a newbie's faults? The "slice" method is obviously NOT thread safe!? But I don't have any idea why.... :(

Thanks in anticipation ;)

void slice(int minY, int maxY) {//Routine now set to do a horizontal "slice" for multhreading  :)
	long double newReal, newImaginary, oldReal, oldImaginary;//real and imaginary parts of new and old
	long double pixelX, pixelY;//real and imaginary parts of the pixel p
	int iter, y, x;
	COLORREF Colour;//Working colour
	for (y = minY; y < maxY; y++) {//Change this to a slice - 180 horizontal lines....
		pixelY = (y - halfheight) / (zoom * halfheight) + moveY;//calculate (Only ONCE per y) the real part of Z, based on the pixel location, zoom and position values
		for (x = 0; x < width; x++) {
			pixelX = 1.5 * (x - halfwidth) / (zoom * halfwidth) + moveX; //calculate the imaginary part of Z, based on the pixel location, zoom and position values
			newReal = newImaginary = 0; //these should start at zero
			for (iter = 0; iter < maxIterations; iter++) {//start the iteration (escape) process
				oldReal = newReal; oldImaginary = newImaginary;//remember value of previous iteration
				newReal = oldReal * oldReal - oldImaginary * oldImaginary + pixelX;//The real and imaginary
				newImaginary = 2 * oldReal * oldImaginary + pixelY;				   //parts are calculated
				if ((newReal * newReal + newImaginary * newImaginary) > 4) {//if the point is outside the circle with radius 2, it is escaping - stop
					break;
				}
			}
			if (iter < maxIterations) {// Escaped?
				Colour = Colours[iter % 306];
			} else {
				Colour = BLACK;   //Else BLACK
			}
			SetPixel(hdc, x, y, Colour);
		}//End of for(x (width loop)
	}//End of for(y (height loop)
}

void Mandelbrot() {
        int maxIterations = 2000;//is about the best for zoom 1?
        long double zoom = 1.0E11;//These are correctly defined as "global" "read" only
	long double moveX = 0.2574028981414269373;
        long double moveY = 0.49283797442133693981;

	begintime = clock();//Start the clock

	thread t1(slice, 0, 1080);//Single thread works

	//thread t1(slice, 0, 180);
	//thread t2(slice, 180, 360);
	//thread t3(slice, 360, 540);//If more than 1 thread
	//thread t4(slice, 540, 720);//activated - corrupt display
	//thread t5(slice, 720, 900);
	//thread t6(slice, 900, 1080);

	t1.join();
	//t2.join();
	//t3.join();
	//t4.join();
	//t5.join();
	//t6.join();

	//slice(0, 180);
	//slice(180, 360);
	//slice(360, 540);//Straight methods
	//slice(540, 720);//not threaded - works
	//slice(720, 900);
	//slice(900, 1080);
}

Had problems submitting this topic :(

[Norm Gunderson]

It could be that the SetPixel function is not thread-safe.

You might also want to try creating a different (sub-)image that each thread will write to (i.e. 6 x 1920x180 images), then recombining those 6 images into the final 1920x1080 image after all of the threads have finished. This kind of approach has worked for me when multi-threading image processing applications in the past (though I am far from being an expert).

[cire]

It could be that the SetPixel function is not thread-safe.

Definitely not thread-safe.

https://social.msdn.microsoft.com/Forums/vstudio/en-US/6398cda5-5393-4b16-b8dc-5bbf8902033f/are-winapi-functions-threadsafe-or-no?forum=vclanguage

[louwin]

Thanks for your responses Norm and cire :)

SetPixel not being thread-safe would explain it.

I wanted to include 2 images - one using a single thread (and perfect) and one using 2 threads but I can't see anyway to include images :( I resized the window to 480*320 to limit the size of window :)

C++ newbie here Norm, don't know how to create and address subimages. I also tried adding "hdc = GetDC(sHwnd);" to "slice" with no effect to the output :(

I'm at a loss how to proceed.

I have played with another program that used SFML in exactly the same situation (multithreading with 8 threads) that worked. This program "wrote" to a separate "sprite" image then "drew" the sprite on the screen and used "image.setPixel"....

I decided NOT to use SFML as it was a lot to carry around for basically a "SetPixel" solution.

I cannot understand how a simple "SetPixel" method could cause such havoc in a multithreading situation. After all, each thread is "drawing" a different pixel in a different "slice".

Moreover, I cannot understand it but if I make the threads overlap -

        thread t1(slice, 0, 300);
	thread t2(slice, 180, 360);

the overlapping area (180 to 300) is perfect....

My head hurts :(

[Norm Gunderson]

An example image class:

class Image
{
   public:
      Image(unsigned int _w, unsigned int _h) : w(_w), h(_h), data(_w*_h) {}

   COLORREF &Pixel(unsigned int x, unsigned int y)
   {
      if(x >= w)
         x = w-1;
      if(y >= h)
         y = h-1;
      return data[y*w + x];
   }

   private:
      Image();  // Private, so can't be used
      unsigned int w, h;
      std::vector<COLORREF> data;
};

In your Mandelbrot function create a 1920x1080 image (e.g. Image myIm(1920,1080);)

Change the slice function arguments to receive a reference to an Image object (e.g. void slice(int minY, int maxY, Image &im))and call it from within Mandlebrot with thread t1(slice, 0, 180, std::ref(myIm));

In the slice function, instead of SetPixel(hdc, x, y, Colour); do im.Pixel(x,y) = Colour;

Then, in Mandlebrot, after all of the threads have completed, you can just loop through all x & y and call SetPixel(hdc, x, y, myIm.Pixel(x,y));

If you still have problems then you might need to create a separate Image for each slice (of just 180 lines each), pass a different image to each slice function call and work out which image should be used for each y value when you're calling SetPixel at the end of Mandlebrot

Hopefully that will help. I must say that when things go wrong with multi-threading it can be a devil to trace what's going on & even to replicate the issue/symptom. All you can do (in my experience) is trawl through the code & make sure that the threads will never access the same data.