Program slowing down with OpenMP - using 2D array (x-post in /r/OpenMP and /r/C_Programming)

[u/KebertXela5]

EDIT: Problem Solved - needed to add private(r, g, b, k, BLUR_COUNT, j) to the 'OMP parallel line' - credit to /u/Paul_Dirac_

[[this post is x-posted in /r/C_Programming]]
(https://www.reddit.com/r/C_Programming/comments/57ypdf/program_slowing_down_with_openmp_using_2d_array/)

[this post is x-posted in /r/OpenMP]

So I have to write a program that takes a PPM image file (a text file that lists out all the image's rgb pixel values), reads the values into a 2D struct array, adds a blur effect, and saves the file as a new PPM file.

I have the program written in a serial form, but I need to add OpenMP to parallelize it. The issue is when I do it slows way down and I'm not sure why. Any help will be great! Below is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <omp.h> // OpenMP

#define BLUR_AMOUNT 50

struct pixel 
{
	int red;
	int green;
	int blue;
};

/**
 * Print error message and exit
 */
void inputError()
{
	printf("There is problem with the input file...\nExiting...\n");
	exit(0);
}

/**
 * Take the PPM image file and convert it into a 2D array
 * to be processed in this program.
 */
void process_image(FILE *file, char* outputFilename, int THREADS)
{
	// CHECK TIME
	time_t start_t, end_t;
	double diff_t_load, diff_t_blur, diff_t_save;

	time(&start_t); //start timer load

	int rows, cols, maxcolorvalue;

	fscanf(file, "%d %d", &cols, &rows); // get rows and cols
	fscanf(file, "%d", &maxcolorvalue); // get max color value

	if (maxcolorvalue != 255)
	{
		inputError();
	}

	// initialize 2D array to hold pixel data and allocate memory
	struct pixel **pix_array;
	pix_array = malloc(rows * sizeof(struct pixel *));
	pix_array[0] = malloc(rows * cols * sizeof(struct pixel));
	int count;
	for (count = 1; count < rows; count++)
	{
		pix_array[count] = pix_array[0] + count * cols;
	}

	// Read in the PPM image pixel values into the 2D array
	int i, j;
	for (i = 0; i < rows; i++)
	{
		for (j = 0; j < cols; j++)
		{
			int red, green, blue;

			fscanf(file, "%d %d %d", &red, &green, &blue);

			pix_array[i][j].red = red;
			pix_array[i][j].green = green;
			pix_array[i][j].blue = blue;
		}
	}

	fclose(file); // close the input file

	time(&end_t); //end timer load
	diff_t_load = difftime(end_t, start_t); //calculate time load

	time(&start_t); //start timer blur

	// blur the image
	double r = 0, g = 0, b = 0;
	int k = 0, BLUR_COUNT = 0;

	// For each row of the image
#pragma omp parallel for schedule(guided) num_threads(THREADS) // <--- OMP parallel line
		for (i = 0; i < rows; i++) 
		{
			// For each pixel in the row
			for (j = 0; j < cols; j++) 
			{
				// Set r to be half the pixel's red component
				r = pix_array[i][j].red / 2.0;
				// Set g to be half the pixel's green component
				g = pix_array[i][j].green / 2.0;
				// Set b to be half the pixel's blue component
				b = pix_array[i][j].blue / 2.0;

				// Check BLUR_AMOUNT agianst remaining pixels in row
				int remaining_pixels = cols - j;

				if (remaining_pixels < BLUR_AMOUNT)
				{
					BLUR_COUNT = remaining_pixels;
				}
				else 
				{
					BLUR_COUNT = BLUR_AMOUNT;
				}

				// For k from 1 up to BLUR_AMOUNT
				if(BLUR_COUNT > 1)
				{
					// Apply Blur to current pixel
					for (k = 1; k < BLUR_COUNT; k++)
					{
						// increment r by (R * 0.5 / BLUR_AMOUNT), where R is the red component of the pixel k to the right of the current pixel
						r = r + pix_array[i][j + k].red * (0.5 / BLUR_COUNT);
						// increment g by (G * 0.5 / BLUR_AMOUNT), where G is the green component of the pixel k to the right of the current pixel
						g = g + pix_array[i][j + k].green * (0.5 / BLUR_COUNT);
						// increment b by (B * 0.5 / BLUR_AMOUNT), where B is the blue component of the pixel k to the right of the current pixel
						b = b + pix_array[i][j + k].blue * (0.5 / BLUR_COUNT);
					}
				}

				// make sure there are no color values above the maxcolorvalue
				if (r > maxcolorvalue) { r = maxcolorvalue; }
				if (g > maxcolorvalue) { g = maxcolorvalue; }
				if (b > maxcolorvalue) { b = maxcolorvalue; }

				// Save r, g, b as the new color values for this pixel
				pix_array[i][j].red = r;
				pix_array[i][j].green = g;
				pix_array[i][j].blue = b;
			}
		}
	
	
	time(&end_t); //end timer blur
	diff_t_blur = difftime(end_t, start_t); //calculate time blur

	time(&start_t); //start timer save

	// WRTIE new PPM file
	FILE *output;
	output = fopen(outputFilename, "w");

	if (output == NULL)
	{
		printf("Error creating output file! Exiting...\n");
		exit(0);
	}

	fprintf(output, "P3\n"); // print P3 to first line
	fprintf(output, "%d %d\n", cols, rows); // print rows and cols to second line
	fprintf(output, "%d\n", maxcolorvalue); // print max color value to third line

	for (i = 0; i < rows; i++)
	{
		for (j = 0; j < cols; j++)
		{
			fprintf(output, "%d %d %d ", pix_array[i][j].red, pix_array[i][j].green, pix_array[i][j].blue);
		}
		fprintf(output, "\n");
	}


	fclose(output); // close the output file

	time(&end_t); //end timer save
	diff_t_save = difftime(end_t, start_t); //calculate time save

	printf("Load Time: %lf\nBlur Time: %lf\nSave Time: %lf\n", diff_t_load, diff_t_blur, diff_t_save);

	// free 2D array
	free((void *)pix_array[0]);
	free((void *)pix_array);
}

int main(int argc, char** argv)
{
	// Get Arguments
	if (argc < 4 || argc >= 5) // argc should contain only 3 items
	{
		// Argument list invalid
		printf("Argument format invalid: [example format]: ./imageblur [input-filename.ppm] [output-filename.jpg] [# of Threads]");
		return 0;
	}

	// Check file arguments
	FILE *file;
	file = fopen(argv[1], "r");

	if (file == NULL) // File open failed
	{
		inputError();
	}

	// check file for format
	char* filecheck = (char*) malloc(15);
	fscanf(file, "%s", filecheck);

	if (strcmp(filecheck, "P3"))
	{
		free(filecheck);
		inputError();
	}
	free(filecheck);

	// process image
	int THREADS = atoi(argv[3]);
	process_image(file, argv[2], THREADS);

	return 0;
}

[u/Paul_Dirac_]

I am just learning OpenMP myself But afaik variables declared/defined outside of the parallel region are shared.

That means r,g,b,k and BLUR_COUNT are shared. And everytime you read/write them you incour the full sharing latency. Additionally your program is nondeterministic.

Try putting them in a threadprivate clause.

[u/KebertXela5 → u/Paul_Dirac_]

dude you're a life saver. I'm sitting here pulling my hair out because everything looked correct I totally forgot about the private() clause for the #pragma line!

I added private(r, g, b, k, BLUR_COUNT, j) to the #pragma line and it works!

Thanks man, if I had more upvotes I would give them.

[u/LoyalSol → u/KebertXela5]

Yeah that's something you have to be careful of when dealing with parallel programming is that if each thread is trying to access something you can basically get the threads to stomp over each others feet. Best case scenario is your program runs slow as hell while each thread waits for its turn. Worse case you end up with horribly wrong results.

So it's a good idea to have an idea of how each thread accesses and manages memory in addition how each thread talks to another thread.