What should I be replacing foreach with?

fcarney

Well, if you are using C++ 11 there is the new standard for:

QList<QString> slist;
for(auto s: slist){
  qInfo() << s;
}

This form works on C++ containers and most Qt list like containers. You can even do temporary lists:

for(auto item: {5,6,2,8}){
}

As long as all items in the braces are of the same type.

Edit: The article that was linked twice does mention this form of the for loop. It basically says replace foreach(a,b) with for(a:b) and you are done.
Example:

foreach(int a, intlist){
}
becomes:
for(int a: intlist){
}

aha_1980

@fcarney please read the link I posted above carefully.

In short: When using Qt containers with range based for loops, make sure the container is const. Otherwise the container may detach which causes a deep copy to happen.

Regards

fcarney

@aha_1980 said in What should I be replacing foreach with?:

please read the link I posted above carefully.

I think I have seen this detach happen. To prevent it copying my data I changed to a list of pointers to iterate over. So at most it only copied the pointer itself. Thanks for clarifying this.

kshegunov

@fcarney said in What should I be replacing foreach with?:

To prevent it copying my data I changed to a list of pointers to iterate over.

Killing mosquitoes with a shotgun, are we? :)
There's this handy function for these cases.

fcarney

I am struggling to see the problem:

#include <QCoreApplication>

#include <QDebug>

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    QStringList list;
    list << QString("hello");
    list << QString("world");

    qInfo() << "org list:" << list;

    // iterate
    for(auto &str: list){
        str += "s";
        qInfo() << str;
    }

    qInfo() << "org list:" << list;

    // How is this case copying anything?
    // Because it calls iterator instead of const_iterator?
    for(const auto &str: list){
        qInfo() << str;
    }

    for(const auto &str: qAsConst(list)){
        qInfo() << str;
    }

    for(auto &str: qAsConst(list)){
        qInfo() << str;
    }

    return a.exec();
}

Where is the copy happening? And no, I saw the copy problem from using {} to group objects. Not from using a loop. That is why I am very confused by this**.

Edit: So is the issue because Qt objects use COW (copy on write)?
https://stackoverflow.com/questions/5346890/what-is-the-difference-between-const-iterator-and-iterator/5346927
While most std containers do not have this issue? Short term, use qAsConst(), long term, move away from Qt container objects? How much overheard is there really in this? Does someone have an example where we can see the overhead issue? Like something takes twice as long to run?

kshegunov

@fcarney said in What should I be replacing foreach with?:

I am struggling to see the problem
Where is the copy happening?

In this example, nowhere. Here's one that's going to trigger it:

QVector<int> someData;
QVector<int> other(someData);

for (int x : other) { //< Detach here
}

Edit: So is the issue because Qt objects use COW (copy on write)?

Yes.

While most std containers do not have this issue?

None of them do. COW is not allowed for the STL containers.

Short term, use qAsConst(), long term, move away from Qt container objects?

No. Use Qt containers, but if you're not going to modify them loop through with an immutable iterator (i.e. use qAsConst with range-based for).

How much overheard is there really in this?

Copying the data is the overhead. STL does that anyway, so in the above example you'd have two copies of the same data if you were to use the STL. Qt is a bit smarter - it copies the data when it needs to, when the data is about the change.

Does someone have an example where we can see the overhead issue? Like something takes twice as long to run?

This should do it:

static constexpr int scale = 1000000, iterations = 1000 * scale;

typedef QVector<int> IntVector;

QElapsedTimer timer;
IntVector data(scale);

timer.start();
for (int i = 0; i < iterations; i++)  {
    IntVector localRef(data);
    IntVector::Iterator iterator = localRef.begin();
}
qDebug() << "Detaching: " << timer.elapsed();

timer.start();
for (int i = 0; i < iterations; i++)  {
    IntVector localRef(data);
    IntVector::ConstIterator iterator = localRef.constBegin();
}
qDebug() << "Non-detaching: " << timer.elapsed();

mmikeinsantarosa

kshegunov,

I'm trying to follow along and I can't find a definition for:
IntVector

Could you elaborate please?
I'm pretty new to C++.

thanks

Christian Ehrlicher

@mmikeinsantarosa said in What should I be replacing foreach with?:

I'm trying to follow along and I can't find a definition for:
IntVector

two lines above: typedef QVector<int> IntVector;

fcarney

@kshegunov said in What should I be replacing foreach with?:

static constexpr int scale = 1000000, iterations = 1000 * scale;

I tweaked your settings a bit:

static constexpr int scale = 200000, iterations = 1 * scale;

I get the following output:

Detaching:  5574
Non-detaching:  10

That is a HUGE difference in time. Like amazingly huge.

Now I am trying to reproduce this with ranged for loops, but am getting zero for the timer output of those versions. Is my code getting optimized out for some reason?:

// detach example
    static constexpr int scale = 200000, iterations = 1 * scale;
    typedef QVector<int> IntVector;

    QElapsedTimer timer;
    IntVector data(scale);

    timer.start();
    for (int i = 0; i < iterations; i++)  {
        IntVector localRef(data);
        IntVector::Iterator iterator = localRef.begin();
    }
    qDebug() << "Detaching: " << timer.elapsed();

    timer.start();
    for (int i = 0; i < iterations; i++)  {
        IntVector localRef(data);
        IntVector::ConstIterator iterator = localRef.constBegin();
    }
    qDebug() << "Non-detaching: " << timer.elapsed();

    int b=0;
    int c=0;
    IntVector other(data);

    timer.start();
    for(auto i: other){
        b += i;
    }
    c=b;
    qDebug() << c;
    qDebug() << "ranged Detaching: " << timer.elapsed();

    timer.start();
    for(auto i: qAsConst(other)){
        b += i;
    }
    c=b;
    qDebug() << c;
    qDebug() << "ranged Non-detaching: " << timer.elapsed();

I get zeros from this as well:

timer.start();
    for(auto &i: other){
        b += i;
        i = b;
    }
    c=b;
    qDebug() << c;
    qDebug() << "ranged Detaching: " << timer.elapsed();

    timer.start();
    for(auto &i: qAsConst(other)){
        b += i;
        //i = b;
    }
    c=b;
    qDebug() << c;
    qDebug() << "ranged Non-detaching: " << timer.elapsed();

kshegunov

@fcarney said in What should I be replacing foreach with?:

That is a HUGE difference in time. Like amazingly huge.

Why would you think that? In the one case you're copying the same data each time you call localRef.begin(), in the other case you're basically doing refCount++ (where refCount is an atomic integer that tracks the number of objects pointing to the same piece of data). So basically we are comparing how much an integer increment weighs against 200000 std::memcpys of a 200000 element array.

Now I am trying to reproduce this with ranged for loops, but am getting zero for the timer output of those versions.

No you're just detaching one single time, which is rather insignificant in regards to time. Take my original example and put the loop into it, to have detaching two objects need to point to the same data.

Is my code getting optimized out for some reason?

No, your benchmark is insufficient. A go with ranged for would look something like this:

static constexpr int scale = 200000, iterations = 1 * scale;

typedef QVector<int> IntVector;

QElapsedTimer timer;
IntVector data(scale);

timer.start();
for (int i = 0; i < iterations; i++)  {
    int sum = 0;
    IntVector localRef(data);
    for (int & j : localRef)
        sum += j;
}
qDebug() << "Detaching: " << timer.elapsed();

timer.start();
for (int i = 0; i < iterations; i++)  {
    int sum = 0;
    IntVector localRef(data);
    for (const int & j : qAsConst(localRef))
        sum += j;
}
qDebug() << "Non-detaching: " << timer.elapsed();

fcarney

@kshegunov said in What should I be replacing foreach with?:

So basically we are comparing how much an integer increment weighs against 200000 std::memcpys of a 200000 element array.

I wasn't entirely sure this was going on. I see how it triggers that now.

I think I was able to see this detach in a range based loop now:

    QElapsedTimer timer;
    typedef QVector<int> IntVector;

    int b=0;
    int c=0;
    static constexpr int scale2 = 200000, iterations2 = 1000 * scale2;
    IntVector data2(iterations2);
    IntVector other(data2);

    timer.start();
    for(auto &i: other){
        b += i;
    }
    c=b;
    qDebug() << "ranged Detaching: " << timer.elapsed();

    timer.start();
    for(auto &i: qAsConst(other)){
        b += i;
    }
    c=b;
    qDebug() << "ranged Non-detaching: " << timer.elapsed();

Its a difference that may or may not be a problem. It really depends upon what your doing I guess. I get:

ranged Detaching:  807
ranged Non-detaching:  505

when running the above code. So even a small detach can make a big difference in running time.

mmikeinsantarosa

@Christian-Ehrlicher said in What should I be replacing foreach with?:

typedef QVector<int> IntVector;

Still new here but I can't seem to declare
typedef QVector<int> IntVector;
anywhere without getting an error in qt creator: "Unknown type name IntVector, typedef name must be an identifier"
I tried putting it in the public: section of the header, ahead of the snip, in open space, in it's own header. Using typedef like this works:
typedef struct {
int a;
int b;
} THINGY;

Christian Ehrlicher

@mmikeinsantarosa said in What should I be replacing foreach with?:

typedef QVector<int> IntVector;

including the header for QVector should help

mmikeinsantarosa

@mmikeinsantarosa
oh, if I reverse parts of the statement, it works, error free, ie;
typedef QVector<int> IntVector;
The variable name needs to be after the type.

mmikeinsantarosa

@mmikeinsantarosa
FWIW, I used parameters: static constexpr int scale = 10000, iterations = 100 * scale;
And the non-detaching iterator was 24 times faster
Detaching: 1247
Non-detaching: 51

So this is a good thing to know!