0

Below is the current code that I am working with. When I comment out the code to run the progress_bar function, code works perfectly as expected with the mandelbrot printed out into a seperate image file. Yet for whatever reason, when I try to include the function it screeches everything to a halt when the rest of the program has finished up. How do I include the progress_bar function without the program locking into a pseudo-deadlock state? Any and all help is appreciated.

#include <cstdint>
#include <cstdlib>
#include <complex>
#include <fstream>
#include <iostream>
#include <vector>
#include <thread>
#include <mutex>

// Import things we need from the standard library
using std::chrono::duration_cast;
using std::chrono::milliseconds;
using std::complex;
using std::cout;
using std::endl;
using std::ofstream;

// Define the alias "the_clock" for the clock type we're going to use.
typedef std::chrono::steady_clock the_clock;


// The size of the image to generate.
const int WIDTH = 1920;
const int HEIGHT = 1200;

// The number of times to iterate before we assume that a point isn't in the
// Mandelbrot set.
// (You may need to turn this up if you zoom further into the set.)
const int MAX_ITERATIONS = 500;

// The image data.
// Each pixel is represented as 0xRRGGBB.
uint32_t image[HEIGHT][WIDTH];

double progress;
bool progressDone = false;
std::mutex locking;
std::condition_variable conditionMet;
int partsDone = 0;

// Write the image to a TGA file with the given name.
// Format specification: http://www.gamers.org/dEngine/quake3/TGA.txt

void progress_bar() {

    std::unique_lock<std::mutex> lock(locking);
    while (!progressDone) {

        //std::this_thread::sleep_for(std::chrono::nanoseconds(100));
        cout << "Current Progress is at: " << progress << "%\n";
        conditionMet.wait(lock);

    }

    cout << "Mandelbrot is finished! Take a look.";

}

void write_tga(const char *filename)
{
    ofstream outfile(filename, ofstream::binary);

    uint8_t header[18] = {
        0, // no image ID
        0, // no colour map
        2, // uncompressed 24-bit image
        0, 0, 0, 0, 0, // empty colour map specification
        0, 0, // X origin
        0, 0, // Y origin
        WIDTH & 0xFF, (WIDTH >> 8) & 0xFF, // width
        HEIGHT & 0xFF, (HEIGHT >> 8) & 0xFF, // height
        24, // bits per pixel
        0, // image descriptor
    };
    outfile.write((const char *)header, 18);

    for (int y = 0; y < HEIGHT; ++y)
    {
        for (int x = 0; x < WIDTH; ++x)
        {
            uint8_t pixel[3] = {
                image[y][x] & 0xFF, // blue channel
                (image[y][x] >> 8) & 0xFF, // green channel
                (image[y][x] >> 16) & 0xFF, // red channel
            };
            outfile.write((const char *)pixel, 3);
        }
    }

    outfile.close();
    if (!outfile)
    {
        // An error has occurred at some point since we opened the file.
        cout << "Error writing to " << filename << endl;
        exit(1);
    }
}


// Render the Mandelbrot set into the image array.
// The parameters specify the region on the complex plane to plot.
void compute_mandelbrot(double left, double right, double top, double bottom, double start, double finish)
{
    for (int y = start; y < finish; ++y)
    {
        for (int x = 0; x < WIDTH; ++x)
        {
            // Work out the point in the complex plane that
            // corresponds to this pixel in the output image.
            complex<double> c(left + (x * (right - left) / WIDTH),
                top + (y * (bottom - top) / HEIGHT));

            // Start off z at (0, 0).
            complex<double> z(0.0, 0.0);

            // Iterate z = z^2 + c until z moves more than 2 units
            // away from (0, 0), or we've iterated too many times.
            int iterations = 0;
            while (abs(z) < 2.0 && iterations < MAX_ITERATIONS)
            {
                z = (z * z) + c;

                ++iterations;
            }

            if (iterations == MAX_ITERATIONS)
            {
                // z didn't escape from the circle.
                // This point is in the Mandelbrot set.
                image[y][x] = 0x000000; // black
            }
            else if (iterations == 0) {

                image[y][x] = 0xFFFFFF;

            }
            else
            {
                // z escaped within less than MAX_ITERATIONS
                // iterations. This point isn't in the set.
                image[y][x] = 0xFFFFFF; // white
                image[y][x] = 16711680 | iterations << 8 | iterations;
            }

            std::unique_lock<std::mutex> lock(locking);
            progress += double((1.0 / (WIDTH*HEIGHT)) * 100.0);
            conditionMet.notify_one();
        }
    }
    partsDone += 1;
}


int main(int argc, char *argv[])
{
    cout << "Please wait..." << endl;

    // Start timing
    std::vector<std::thread*> threads;
    the_clock::time_point start = the_clock::now();

    std::thread progressive(progress_bar);

    for (int slice = 0; slice < 2; slice++) {

        // This shows the whole set.
        threads.push_back(new std::thread(compute_mandelbrot, -2.0, 1.0, 1.125, -1.125, HEIGHT * (slice / 2), HEIGHT * ((slice + 1) / 2)));

        // This zooms in on an interesting bit of detail.
        //compute_mandelbrot(-0.751085, -0.734975, 0.118378, 0.134488, 0, HEIGHT/16);

    }

    // Stop timing

    for (std::thread* t : threads) {

        t->join();
        delete t;

    }

    if (partsDone == 2) {

        progressDone = true;

    }

    progressive.join();

    the_clock::time_point end = the_clock::now();

    // Compute the difference between the two times in milliseconds
    auto time_taken = duration_cast<milliseconds>(end - start).count();
    cout << "Computing the Mandelbrot set took " << time_taken << " ms." << endl;

    write_tga("output.tga");

    std::this_thread::sleep_for(milliseconds(3000));

    return 0;
}```
Kitso
  • 51
  • 5
  • Put `conditionMet.notify_one();` before the call to `progressive.join()`. I've ommitted the actual prints to be able to run in an online compiler : http://coliru.stacked-crooked.com/a/34e2d4eb52a67cc6 . – Nikos Athanasiou May 27 '22 at 16:53
  • But I am doing that in the compute_mandelbrot function, in fact since the function is run twice it should be doing it twice. Or do it need to notify it once I am about to join the thread as well? – Kitso May 27 '22 at 16:55
  • You do that but `progressDone` is still false and your progress bar loop keeps executing. By the time you set the flag, no one notifies that thread again – Nikos Athanasiou May 27 '22 at 17:00
  • `partsDone` is not atomic, not protected by a mutex and not synchronized between threads including the main thread. Are you sure your code isn't blocking there? – Goswin von Brederlow May 27 '22 at 17:02
  • Ah right thank you, though I have put in `conditionMet.nofify_one();` before the call and the program does still appear to still lock up even after it was notified that the boolean was set to true. – Kitso May 27 '22 at 17:02
  • @GoswinvonBrederlow he has a data race there, but not the blocking location. http://coliru.stacked-crooked.com/a/d68ff2379e2a64a0 (attaching a thread sanitizer output) – Nikos Athanasiou May 27 '22 at 17:03
  • @GoswinvonBrederlow `partsDone` should be protected by a mutex from the unique mutex above it isn't it? – Kitso May 27 '22 at 17:03
  • But the race could mean `progressDone` is never set to `true`. – Goswin von Brederlow May 27 '22 at 17:03
  • @kitso There is no lock or mutex in the scopes where `partsDone` is used. – Goswin von Brederlow May 27 '22 at 17:05
  • This would all look much simpler if you changed `progress` to `std::atomic` and had your progress printer just load the variable in say `100ms` intervals (and printed on top of the previous line). Then all you'd need would be the `progressDone` flag instead of locking and printing for every modification of the progress value – Nikos Athanasiou May 27 '22 at 17:05
  • with `progressDone` also atomic – Goswin von Brederlow May 27 '22 at 17:06
  • @GoswinvonBrederlow `progressDone` is a boolean, by definition load/store operations are atomic there, and inside the program it's only set to `true` once. It just bloats the sanitizer reports and makes difficult to identify real probolems, so yeah I'd also make it a boolean, but it's not the cause of actual problems – Nikos Athanasiou May 27 '22 at 17:07
  • @NikosAthanasiou Well when trying to chance `progress` to `std::atomic` the += part of the program is throwing an exception that operand types do not match. – Kitso May 27 '22 at 17:10
  • @Kitso Do you mean "compilation error" instead of throwing an exception? – Nikos Athanasiou May 27 '22 at 17:11
  • Well compilation error yes, but it has the red line over the += in the code this is what the full error says: `no operator "+=" matches these operands: operand types are std::atomic += double` – Kitso May 27 '22 at 17:13
  • @Kitso I think I've spent enough time to post an answer. – Nikos Athanasiou May 27 '22 at 17:19
  • Ah okay then, I'll be sure to mark it as helpful since you have been a great help for me so far. – Kitso May 27 '22 at 17:20
  • @NikosAthanasiou load/store of a bool might be atomic but that doesn't make it have a memory barrier. The store will not be observable by other threads in any coordinated way. – Goswin von Brederlow May 27 '22 at 17:33
  • @GoswinvonBrederlow In atomics you can define various memory orders, that doesn't mean it's a race condition. Think of the plain bool as an atomic operation with `std::memory_order_relaxed` https://en.cppreference.com/w/cpp/atomic/memory_order#Relaxed_ordering – Nikos Athanasiou May 27 '22 at 17:39
  • @GoswinvonBrederlow But you are correct about the `partsDone`. This thing can cause a problem and should atomic or mutex protected – Nikos Athanasiou May 27 '22 at 17:43
  • @NikosAthanasiou Exactly. `std::memory_order_relaxed` is not a synchronization operation. And that's the part you need for `partsDone` that is currently missing. – Goswin von Brederlow May 27 '22 at 17:48
  • @GoswinvonBrederlow I'm not arguing about the `partsDone` that's an `int`. `++` on ints is anyways non atomic. When I talk about atomic load/store and being safe even if not declared atomic I'm only arguing about `progressDone`. If you look above I've already agreed with you that `partsDone` should be `std::atomic_int` – Nikos Athanasiou May 27 '22 at 17:51
  • sorry, I mend for `progressDone`. The part that's missing is the synchronization. Without that the progress bar will likely not work on ARM. – Goswin von Brederlow May 27 '22 at 17:52

1 Answers1

1

The reason for your non-termination is that no-one notifies the progress bar thread after all the work has finished. Add conditionMet.notify_one(); before the call to progressive.join(). I've ommitted IO to be able to run in an online compiler in the following Demo. Also (as @GoswinvonBrederlow mentions in the comments) make sure to turn partsDone into std::atomic because if >1 threads call partsDone += 1 you'll end up with undefined results and in turn you won't be able to tell if you program is finished.

This would all look simpler if you changed progress to std::atomic and had your progress printer just load the variable in say 100ms intervals (and printed on top of the previous line). Then all you'd need would be the progressDone flag instead of locking and printing for every modification of the progress value. You can see in the following Demo that this runs with zero thread sanitizer warnings. Make sure to adjust the printing interval. This change drops the runtime from ~10.7s to 7s, even though that is just an indication - it's not kosher to time your programs with the thread sanitizer on.

Nikos Athanasiou
  • 29,616
  • 15
  • 87
  • 153
  • Well this does help a lot thank you, but I am still having the problem with progress and += even though my line looks exactly like yours does in the code. Wait, fixed it by just making it into = but it adds the formula onto itself. – Kitso May 27 '22 at 17:32
  • @Kitso Did you `#include `? – Nikos Athanasiou May 27 '22 at 17:33
  • I did, but I managed to fix the issue, but still thank you for the assistance. – Kitso May 27 '22 at 17:34