QList<QObject*> and Memory Leak Issue

Dong

Hi !

I got a problem when manage large data set in QList<>
The following code cause an Performance & Memory Leak Issue.

If I not call : qDeleteAll(listItems);
It cause Memory Leak
But when call it, the process to free memory related to those pointer is very slow... (about 1.5s per 1000 items)

QList<QObject*> listItems;
    for (int i = 0; i < 1000000; ++i) {
        DynamicObject* item = new DynamicObject(AdsProvider::FACEBOOK, "Flexibles");
        item->setProperty("FlexibleId", i);
        item->setProperty("LocalCampaignId", i);
        item->setProperty("CampaignId", i);
        item->setProperty("State", i);
        item->setProperty("Name", QString("Items %1").arg(i));
        item->setProperty("Excluded", i%2 == 0);

        listItems.append(item);
        if (i%1000 == 0)
        {
            qDebug() << "Clear Items: " << QDateTime::currentDateTime().toString("yyyy-MM-dd hh:mm:ss zzz");
            qDeleteAll(listItems);
            listItems.clear();
        }

        //CommonBusiness::Save(AdsProvider::FACEBOOK, "371526396387397", item);
    }

Is this an QT Issue ?!
How can I avoid it?
Is there any BEST PRACTICES or IMPORTANT NOTE to "Avoid Memory Leak in Qt" ?

micland

3 thoughts:

If you want to delete all your items, you have to call qDeleteAll() (or delete each item manually in a separate loop) - QList does not delete your items because it's not the owner of the items (concrete: the list hold just a bunch of pointers and does not own the memory where the pointers point to - another approch is to hold the items on the stack so the list will hold the items and not just the pointers, but that's not possible for QObject instances - they shall be held on the heap, see http://doc.qt.io/qt-5/qobject.html#no-copy-constructor-or-assignment-operator).
If you add so much items try to call listItems.reserve(1000000) at first to prevent frequently resizing of the internal structure.
If your DynamicObject class inherits QObject you can pass another QObject instance as parent to the constructor so your item will be automatically deleted when the parent object gets deleted (see http://doc.qt.io/qt-5/objecttrees.html). But QList does not inherit from QObject so it can't be used as parent.

? Offline

Hi! First of all @micland is right about qDeleteAll(). Regarding performance: Of course delete is a costly operation but 1.5 seconds for 1000 objects seems to be a lot. Must be very heavy objects and a very slow computer. How did you meassure this? One simple way to print the time (in milliseconds) it took to delete 1000 objects is:

if (i%1000 == 0)
{
    QTime time;
    time.start();
    qDeleteAll(listItems);
    qDebug() << QString("%1").arg(time.elapsed()); // milliseconds
    listItems.clear();
}

micland

Oh I missed the thing that you clear the list after adding 1000 items, so don't call listItems.reserve(1000000) but listItems.reserve(1000).
As @Wieland said 1.5s for 1000 items to delete is a lot. And the measured time you're printing out also contains the creation and initialization of the item instances. Are you sure that the deletion is taking the time and not the initialization? If so, can you post the implementation of the destructor of your DynamicObject item class? I guess that there's anything time consuming... (or try to add just QObject instances instead of DynamicObject - does it take the same time to delete them?)

VRonin

This post is deleted!

Dong

@micland

1st: I use DynamicObject with Dynamic Properties to contain Unknown Data Structure get from anonymous Service.
2nd: I need to use a list of DynamicObject on QML, that why I have to use QList<QObject*>
My related topic here

@Wieland

I mean-sure all "create - add - delete - clear" process in above sample code. May be I will break it out.
If I change each Item instantiate with QObject* item = new QObject();
It only take 0.4 - 0.5s (May be Data conversion between QObject & DynamicObject is take time.

@VRonin
- I'm already defined Destructor in DynamicObject (in fact: It doesn't have any predefined or instantiated properties so Destructor is not necessary )

micland

@Dong said:

I mean-sure all "create - add - delete - clear" process in above sample code. May be I will break it out.

If I change each Item instantiate with QObject* item = new QObject();
It only take 0.4 - 0.5s (May be Data conversion between QObject & DynamicObject is take time.

Well if the operation is faster when using QObject instances than the time consuming part happens in your own constructor or destructor implementation. Since your QList is holding only pointers and not the items directly there's no "data conversion" that takes time (or try to use QList<DynamicObject*> - but that should not influence the timing).

Dong

"It definitely slow on Instantiate DynamicObject."
After investigate, I found that Signals/Slots/Functions in DynamicObject also affect the Instantiate process.

I am create 1000 items
- Without any Signal/Slot/Function : it only take 200ms
- With 5 Q_INVOKABLE functions, 1 Signal, 1 Slot : it take 950ms

And my DynamicObject have about 10 Q_INVOKABLE functions, 2 Signals, 2 Slots

Below is the causes For Memory Leak Issues (Please correct if I'm wrong)

Delete pointer when not using it.
Use Destructor to clear properties.
Use Parent - Child Tree to manage delete Child Objects automatically when Parent Object deleted.

kshegunov

@Dong

After investigate, I found that Signals/Slots/Functions in DynamicObject also affect the Instantiate process.

I am create 1000 items
Without any Signal/Slot/Function : it only take 200ms
With 5 Q_INVOKABLE functions, 1 Signal, 1 Slot : it take 950ms
And my DynamicObject have about 10 Q_INVOKABLE functions, 2 Signals, 2 Slots

This is very suspicious. 1k objects is nothing(!). Also I don't see how you'd get 5 fold increase by using 5 invokables. The macro expands to nothing and moc generates only a few bytes for it. So, as @micland already asked, how do you measure those times?

Delete pointer when not using it.
Use Parent - Child Tree to manage delete Child Objects automatically when Parent Object deleted.

This will keep you free of memory leaks for the most part, yes.

Use Destructor to clear properties.

Never call the destructor directly unless you know what you're doing.

Kind regards.

micland

@Dong said:

Use Destructor to clear properties.

In addition to the notes of @kshegunov, what exactly do you mean with "clear properties"?
The properties specified using setProperty(...) are owned and managed by QObject so there is no need to clear them. You only have to clean up in your destructor what you have acquired in your constructor (memory, resources, etc.) - but that's common C++, nothing Qt specific. Qt helps you to reduce the the required clean up, for example with the mentioned object tree (a parent instance owns and deletes its child instances with parent/child relation here in a linked way, not an inherited way).

But using Qt does not require more action to avoid memory leaks than common C++ does (usually).

Dong

Thanks you for all comment and suggestions.

Because I need to "binding" between Qml's Properties & Dynamic Properties. So, I store a mapping in my object.
Do I need to clean up that map in Destructor?

Here is my logged time use qDebug():
Action: "Start time"
Create Items: "2016-06-08 16:15:45 553"
Delete Items: "2016-06-08 16:15:46 062"
Clear List: "2016-06-08 16:15:46 095"
Create Items: "2016-06-08 16:15:46 095"
Delete Items: "2016-06-08 16:15:46 692"
Clear List: "2016-06-08 16:15:46 737"
Create Items: "2016-06-08 16:15:46 738"
Delete Items: "2016-06-08 16:15:47 686"
Clear List: "2016-06-08 16:15:47 735"
Create Items: "2016-06-08 16:15:47 736"
Delete Items: "2016-06-08 16:15:48 693"
Clear List: "2016-06-08 16:15:48 733"
Create Items: "2016-06-08 16:15:48 741"
Delete Items: "2016-06-08 16:15:49 724"
Clear List: "2016-06-08 16:15:49 768"

kshegunov

@Dong

Here is my logged time use qDebug()

This information is useless. You can't benchmark code like that. You need to repeat the block a few times and you need an okay timer (QElapsedTimer for example, QDateTime::currentDateTime() will simply not cut it!). Also you need to make sure you have the same (or almost the same) conditions on every benchmark iteration.

Because I need to "binding" between Qml's Properties & Dynamic Properties. So, I store a mapping in my object.

Show us how you do that, please. QML's properties are already dynamic properties, so I can't help but wonder what mapping you need exactly.

Dong

@kshegunov

About the benchmark thing, I use qDebug to print out the start time of each process.
Above log sample is process with 5000 items (5 times * 1000)
But Just forget about the benchmark thing ...

here is my other post about my situation.
Currently I only can auto binding one way from View Model (C++) to View (Qml)

Here is my DynamicObject definition

class DynamicObject : public QObject
{
    Q_OBJECT
private:
    QObject* getProperty(const QStringList &propPath) const;
    void CopyObject(QObject * fromObj, QObject * toObj) const;

    // PROPERTIES
    QMap<QString, QVariant> bindingList;                //Mapping between 1 bindingKey to 1 bindingInfo
    QMap<QString, QStringList> cppToQml;                //Mapping between 1 cppProperty to multiple qmlProperties
    QList<QString> bindedChildObjects;                  //List of child object had propertyChanged signal - slot connected

    void removeBindingByQmlProperty(QString objKey, QString propName);
public:

    FormMetaData formMetaData;
    void ClearBinding();

    int getIndexFromPropertyName(QString &propName) const;
    // CONSTRUCTORS
    DynamicObject(QObject *parent = 0);
    DynamicObject(AdsProvider::EnumAdsProvider engine, QString objType);

    // PROPERTIES
    AdsProvider::EnumAdsProvider Engine;
    QString ObjectType;

    // FUNCTIONS
    Q_INVOKABLE void autoBinding(QObject* qmlObj, QString qmlProp, QObject* cppObj, QString cppProp);
    Q_INVOKABLE QObject* thisContextObject();

    Q_INVOKABLE QVariant binding(const QString &propName);
    //===== Normal value Set function =====
    Q_INVOKABLE void setValue(const QString &propPath, const QVariant &value);

    //===== QList functions =====
    ///Update a list item with new item (propPath must point to an item by index)
    Q_INVOKABLE void updateListItem(const QString &propPath, DynamicObject *obj);
    Q_INVOKABLE void insertListItem(const QString &propPath, DynamicObject *obj);
    Q_INVOKABLE void removeListItem(const QString &propPath);
    Q_INVOKABLE void clearListItem(const QString &propPath);
    Q_INVOKABLE int indexOfItem(const QString &propPath, const QVariant &item, const QString &propToCompare);

    bool event(QEvent *event);
    void CopyTo(QObject *toObj);
signals:
    void propertyChanged(QObject* contextObject, QVariant propName);
public slots:
    void onChildPropertyChanged(QObject* cppObj, QVariant propName);
    void onQmlObjectDestroyed(QObject * obj = 0);
};

About ModelData pointer, I'll construct it will ViewModel pointer as parent. it should be work.

May be I'll investigate more on this problems & post the result later.

Thanks you all !!!

kshegunov

@Dong

About the benchmark thing, I use qDebug to print out the start time of each process.

Yes indeed. I meant that you should average a few runs to get a reasonable estimation for how much time it takes. And also QDateTime is not really meant to give you real precise timing, it's intended for dates and time (ordinary clock time).

Currently I only can auto binding one way from View Model (C++) to View (Qml)

To be absolutely candid my knowledge of QML is really rudimentary, but isn't it this how it was intended in the first place?

Kind regards.

Dong

@kshegunov
There is a reason why I can't use QT Property Binding (It only support predefined Property using Q_PROPERTY script).
Please take a look at my topic about Auto Binding 2 ways with Dynamic Property
But Binding is an other topic.

About "Object's pointers & Memory"
I also take an advise from a Pro in C++ & memory management.
His solution is implement a Factory Pattern to Create/Delete Object's pointers
(also free memory when an instance had no pointer to it.)
(I think I'll give it a try)