Loading data in a thread

[Cruz]

As I said at the very beginning, I don't see the point of all this discussion, there is a simple rule of thumb that accessing non-thread-safe data from more than one thread requires proper synchronisation.

And my reply to that is still the same. I want to understand why the problem occurs and not just stupidly follow a rule of thumb. And since the gui thread is only reading, I don't see how the data in the QList can be corrupted. If accessing an implicitely shared container by the [] operator can cause a copy of the data, then that would be a valid reason why it breaks. But it would be nice to have stronger confirmation than "it's not declared thread-safe". It's already not thread-safe if the reading is not guaranteed to work. But the reading is not the problem, the writing is. I will invest some time now and try to extract the relevant portion of the code, so that you can see it.

[wysota]

And my reply to that is still the same. I want to understand why the problem occurs and not just stupidly follow a rule of thumb.

Be my guest, dive in: http://qt.gitorious.org/qt/qt/trees/4.7/src

I don't see how the data in the QList can be corrupted.

The fact is it gets corrupted. So either you are wrong that the data can't be corrupted when one thread is reading it and the other is writing it or you are wrong that one thread is reading it and the other is writing it. I still have not seen your code, so I can only guess that somehow somewhere (maybe unwillingly) you are making a copy of the object which directs you to the path I was describing.

If accessing an implicitely shared container by the [] operator can cause a copy of the data, then that would be a valid reason why it breaks. But it would be nice to have stronger confirmation than "it's not declared thread-safe".

Ok, let's make my hint less subtle: Show us your code!

[Cruz]

So here is the code extract as it was before the mutex.

// One frame of data.
struct Frame
{
    double x;
    double y;
    double z;
}
 
class FileLoader
{
    // The data structure. A QList of Frame structs.
    QList<Frame> data;
}
 
// This is the loader thread.
void FileLoader::run()
{
    Frame f;
 
    QFile file("data.dat");
    file.open(QIODevice::ReadOnly);
    QDataStream in(&file);
 
    int frameCount = 0;
    in >> frameCount;
    for (int i = 0; i < frameCount; i++)
    {
        in >> f.x;
        in >> f.y;
        in >> f.z;
 
        // Here is where a new frame is appended to the QList.
        data << f;
 
        if (i % 1000 == 0)
            emit fileLoadingProgress(100*i/frameCount);
    }
}
 
 
// The GUI class.
class GUI
{
    FileLoader loader; // It has the FileLoader.
    Frame* currentFrame; // And a pointer to the frame currently displayed on the screen.
    double t; // This is a time parameter that is mapped to the data index.
    double tscale; // This influences how fast the time grows (so the speed of the animation).
}
 
GUI::GUI(QWidget *parent)
{
    loader.start(); // It starts the FileLoader thread in the constructor.
    t = 0.0;
    tscale = 1.0;
}
 
// The animate function is periodically called by a timer. 
// It changes the currentFrame pointer by mapping the time t to a data index.
void GUI::animate()
{
    if (t + tscale < loader.data.size()-1)
    {
        t = t + tscale; // t is a double so that it can grow with arbitrary speed.
 
        currentFrame = &(loader.data[(int)t]); // <-- currentFrame pointer assignment.
    }
 
    draw();
}

// One frame of data.
struct Frame
{
    double x;
    double y;
    double z;
}

class FileLoader
{
    // The data structure. A QList of Frame structs.
    QList<Frame> data;
}

// This is the loader thread.
void FileLoader::run()
{
    Frame f;
	
    QFile file("data.dat");
    file.open(QIODevice::ReadOnly);
    QDataStream in(&file);

    int frameCount = 0;
    in >> frameCount;
    for (int i = 0; i < frameCount; i++)
    {
        in >> f.x;
        in >> f.y;
        in >> f.z;

        // Here is where a new frame is appended to the QList.
        data << f;

        if (i % 1000 == 0)
            emit fileLoadingProgress(100*i/frameCount);
    }
}


// The GUI class.
class GUI
{
    FileLoader loader; // It has the FileLoader.
    Frame* currentFrame; // And a pointer to the frame currently displayed on the screen.
    double t; // This is a time parameter that is mapped to the data index.
    double tscale; // This influences how fast the time grows (so the speed of the animation).
}

GUI::GUI(QWidget *parent)
{
    loader.start(); // It starts the FileLoader thread in the constructor.
    t = 0.0;
    tscale = 1.0;
}

// The animate function is periodically called by a timer. 
// It changes the currentFrame pointer by mapping the time t to a data index.
void GUI::animate()
{
    if (t + tscale < loader.data.size()-1)
    {
        t = t + tscale; // t is a double so that it can grow with arbitrary speed.

        currentFrame = &(loader.data[(int)t]); // <-- currentFrame pointer assignment.
    }

    draw();
}

To copy to clipboard, switch view to plain text mode 

Btw, I did not say that the mutex solved the problem. I just said I put it in and it didn't slow anything down. I need to observe the software for a while to see if the broken frames still occur.

[wysota]

As far as I can see your animate() method uses the list as an lvalue (or whatever it was called) so it's likely the list is copied there. You can try using the at() method instead but it will return a copy of the item in the list which will make your currentFrame pointer invalid (as you return a pointer to a temporary object).