Loading data in a thread

[Cruz]

Ok I think I understand. Is it because the [] operator is overloaded and might cause a copy of the whole data, because one thread is reading it and another one is writing it? And then what happens is what Wysota said?

If this is true, the problem would vanish if I used a plain old array of structs instead of the QList, right?

[wysota]

If this is true, the problem would vanish if I used a plain old array of structs instead of the QList, right?

Not necessarily. Adding new items to the list may not be atomic so one of your threads (the reading one) may crash your program.

[Cruz]

I was thinking of a fixed size array of structs, not a list in this case. And the structs don't contain any pointers. So the worst thing that can happen is that the reading thread gets bogus data when trying to access elemets, that have not been written yet.

[wysota]

First you should ask yourself a question - what do you want to achieve exactly. I'm not sure why you are writing to the structure and want to read it at the same time. I'm assuming that you are probably loading data from some file and want to show the user a "live" representation of the structure as it gets filled. I don't understand though why are you so reluctant to use a mutex, it seems a perfect solution here. If you add a bit of smart logic around it, the final effect should be really good.

[Cruz]

I'm assuming that you are probably loading data from some file and want to show the user a "live" representation of the structure as it gets filled.

Precisely.

I don't understand though why are you so reluctant to use a mutex, it seems a perfect solution here. If you add a bit of smart logic around it, the final effect should be really good.

I was a little bit reluctant, because people say it slows things down. But mostly I wanted an explanation why it makes sense, because I didn't know that reading from the QList can trigger copying. The mutex is in place now and there is no noticable change in the performance. But now it bugs me that in the background the data structure gets copied thousands of times even though it's totally unnecessary.

[wysota]

I was a little bit reluctant, because people say it slows things down.

Only when threads try to enter the critical section at the same time but even if they do, you wouldn't even notice the slowdown. You get more slowdown by refreshing the gui thread too often.

But mostly I wanted an explanation why it makes sense, because I didn't know that reading from the QList can trigger copying.

I didn't say that.

The mutex is in place now and there is no noticable change in the performance. But now it bugs me that in the background the data structure gets copied thousands of times even though it's totally unnecessary.

So don't copy it.

[Cruz]

I didn't say that.

So then why does my data get corrupted in the first place?

[wysota]

I haven't seen your code, how could I answer your question?

[Cruz]

Ok, so the whole thread was pointless so far?

Can you please answer just this one specific question, that I already asked several times. If one thread is only reading from a QList and another thread is only writing, can it happen that at least one element of the QList gets corrupted for good? And again I emphasize that reading a half written element is NOT the point. There are errors in the data that remain even after the writing thread has finished its job.

[wysota]

Ok, so the whole thread was pointless so far?

I can't tell you what is wrong with your code because I didn't see it. I can only tell you what might be wrong with your code based on your description and that's what I did. Since adding a mutex solved the problem it is more likely that I was correct but I can't be sure because I haven't seen your code.

If one thread is only reading from a QList and another thread is only writing, can it happen that at least one element of the QList gets corrupted for good?

Yes, I believe so since QList is not declared thread-safe for a reason.

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.

[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).