Simple multithreading C project

Question

I'm fairly new to C, and this is my first time using pthread. I am pretty sure I implemented it correctly, however I'd like to verify it is indeed executing the tasks parallel instead of sequentially. Here is my main.c code:

main.c

#include "frequency.h"

/**
 * The main function. Runs {@code getMedianWord()} in parallel threads for each file, 
 * then outputs the sorted elements in FileArray file_array.
 */
int main(int argc, char **argv) {
    int actual_args = argc - 1;
    pthread_t tid[argc];

    for (int i = 0; i < actual_args; ++i)
        pthread_create(&tid[i], NULL, getMedianWord, (void *) &argv[i + 1]);

    for (int j = 0; j < actual_args; ++j)
        pthread_join(tid[j], NULL);

    qsort(file_array,
          (size_t) actual_args,
          sizeof(FileArray),
          (int (*)(const void *, const void *)) file_cmp
    );

    for (int k = 0; k < actual_args; ++k)
        printf("\n%s %d %s",
               file_array[k].filename,
               file_array[k].num,
               file_array[k].median_word
        );

    pthread_exit(NULL);
}

frequency.c

...
void *getMedianWord(void *vargp) {
    char *filename = *(char **) vargp;
    WordArray word_array[MAX_WORDS];

    FILE *fp = fopen(filename, "r");
    char word[101];

    int n = 0;
    while (!feof(fp)) {
        fscanf(fp, "%s", word);
        insert_word(word_array, &n, word);
    }

    insert_arr(file_array, filename, n, word_array);

    pthread_exit(NULL);
}

The intended code to be executed in parallel is the function getMedianWord, which adds a result to a global array of structs (file_array), and terminates via pthread_exit(NULL).

Please let me know if this is actually using threads properly, thanks!

PS. If you're interested in seeing the rest of my code, here it is.

Answer 1

I agree with commenter @vnp, who says, "The question would greatly benefit from adding getMedianWord code directly. As posted, it is on the verge of being closed as hypothetical." All the interesting stuff is going on in getMedianWord.

TLDR: yep, your use of pthread_create and pthread_join looks fine.

However, I couldn't be sure of that without knowing the declaration of getMedianWord, for which I had to click through to frequency.c:

void *getMedianWord(void *vargp) {
    char *filename = *(char **) vargp;

Okay, this is fine.

---

However, you could do better by passing the char pointer itself as your void* argument, instead of passing the address of the pointer. That is, if you rewrote your getMedianWord function —

void *getMedianWord(void *vargp) {
    const char *filename = vargp;

— then you could rewrite your main loop correspondingly —

for (int i = 1; i < argc; ++i) {
    pthread_create(&tid[i], NULL, getMedianWord, argv[i]);
}

Notice that I've made two other cosmetic changes here. First, I put braces around every compound statement; see goto fail for why. Second, I took your complicated actual_args/i + 1 logic and turned it into an idiomatic loop running over the half-open range [1, argc). Use the most common idioms you can! It saves your reader some brain cells.

The functional change here is replacing &argv[...] with simply argv[...]. (The cast to void* is not required in C. And likewise the cast back from void* inside getMedianWord is not required in C, although it would be required if you wanted to port this code to C++.)

---

I won't very closely review the rest of getMedianWord because you didn't post it.

It is definitely not thread-safe, though; there's a data race on file_index. Look up the _Atomic keyword.

---

I notice that getMedianWord spends most of its time reading from files, which means that it's not likely to parallelize very well. You'll end up being bounded by the speed of your file-reading. Splitting up those reads into different threads doesn't achieve any speedup; in fact it probably wastes time because now you have to wait for a context-switch between each pair of reads. (OTOH, the files will be buffered at the stdio level and also probably brought into memory at the OS level, so maybe I'm wrong about its hurting. I think I'm right about its not helping, though.)

Try reading all the files into memory first, and then splitting up just the computational processing of those files into multiple threads.

In other words, think of "the filesystem" as a contended resource which will destroy your parallelism if everyone's trying to access it at once. (Also think of the malloc/free heap and stdout as contended resources. Looks like you're doing well on those fronts.)

---

Your WordArray word_array[MAX_WORDS]; is a variable-length array, which might blow up your (thread's) stack if MAX_WORDS is increased to, say, 2000000 / sizeof(WordArray) — that is, if MAX_WORDS * MAX_STRING >= 2000000. So watch out for that.