[Development] leak in QMetaObject?

[André Somers]

Op 19/07/2016 om 18:06 schreef Olivier Goffart:
> On Donnerstag, 14. Juli 2016 17:45:58 CEST Thomas Senyk wrote:
>> On 14.07.2016 17:18, André Somers wrote:
>>> Op 14/07/2016 om 17:10 schreef Olivier Goffart:
>>>> On Donnerstag, 14. Juli 2016 16:33:26 CEST Thomas Senyk wrote:
>>>>> Hi,
>>>>>
>>>>> I lately wanted to validate that a connecting&disconnecting to a lambda
>>>>> will not leak the lambda object .. and found that more then that is
>>>>> leaked.
>>>>>
>>>>> Here is my example: http://paste.ofcode.org/36dmWCT8ddF6Zaqb9csKZNv
>>>>>
>>>>> After the 10million connections are executed and disconnected (all of
>>>>> them a successful), I assume a drop in memory consumption and more
>>>>> importantly a drop of cpu load to close to 0%.
>>>>>
>>>>> I see neither one nor the other. I see continues 100% consume with no
>>>>> memory consumption drop what so ever.
>>>>>
>>>>> I did a quick peak into QMetaObject::activate and it seems no clean up
>>>>> happened .. the list returned in
>>>>>
>>>>>     list = &connectionLists->at(signal_index);
>>>>>     (line #3660 in 5.6 checkout from today)
>>>>>
>>>>> seems rather long! (10million?)
>>>>>
>>>>> However all "if (!c->receiver)" shows no receiver and therefor will
>>>>> continue right away .. at least
>>>>>
>>>>> Is this expected? A bug? ... I'm happy to fill a bug report, I just
>>>>> wanted to get some feedback, maybe I'm just doing something wrong
>>>> The lambda object and its capture is destroyed by the call to
>>>>
>>>>     slotObj->destroyIfLastRef()
>>>>
>>>> The internal data structre (QObjectPrivate::Connection) is deleted by
>>>> QObjectPrivate::cleanConnectionLists which is actually only called when
>>>> a new connection is made to this object.
>>>>
>>>> I don't think there is a leak, but it is true that some memory is only
>>>> free'ed
>>>> when you add new connection or that the object is destroyed. (but the
>>>> lambda
>>>> object is destroyed right after the disconnection)
>> Then this:
>> http://paste.ofcode.org/37rxyitynhEqi5gira88hR9
>>
>> should "fix" it? .. but I still see 100% cpu and same memory consumption?
> As thiago says, your thing still does not work because when you connect from a
> signal connected to the same object, the list is inUse and will not be
> cleaned.
>
> for (int i = 1; i <= 10000000; i++) {
>   // ... your code
> }
> QTimer::singleShot(200, // so it's after the last disconnect
>    [] { QObject::disconnect(QObject::connect(t, &QTimer::timeout, []{})); });
>
> So the connect happens after everything was disconnected.
>
> In practice you probably don't need to use this workaround, unless you always
> add new connection from a slot connected to a signal of the object you
> continue to connect and disconnect to. Is that what you are doing?
>
> It is true that we could consider trying to clean the connection list after
> each signal emission if it is dirty.
> We don't want to do it after each disconnect because this can turn a O(n)
> algorithm into a O(n^2)   (cleaning the list is O(n), if we have to do it for
> every disconnect, that n*O(n))
>
Did anyone ever measure in practice what the distribution in values for 
n is here? My *guess* is that it won't be all that large. I expect a 
distribution with a spike at n=1 or 2, and then a dropoff with a very 
long tail, perhaps spiking at a bit larger number when using QML for all 
the propertybindings going on. Big-O notation doesn't make a lot of 
sense if the n is small, it only starts to have an impact if n grows. I 
would be suprised if the impact of the list being a single-linked list 
and thus having to skip through memory to get from node to node was not 
bigger than any "big-O" benefits you gained from doing it this way in 
the first place.

On the other hand, you'd have to consider the likelyhood of another 
connect hapening after a disconnect. If that is nearly 1, then sure, 
that may be a good time to clean up. But if it is closer to 0 in reality...

Note that if you are worried of somebody disconnecting in a loop or 
something: simply make sure to have a fast path. A disconnect 
(ptrSender, 0, ptrReceiver, 0) obviously doesn't need a cleanup after 
every individual disconnect...

André