Re: sip: Accessing members results in incorrect/missing data

Phil Thompson-5 wrote
> On 21/01/2020 17:50, Phil Thompson wrote:
>> On 16/01/2020 20:52, MatthijsBurgh wrote:
>>> I have done some more investigating.
>>> 
>>> As mentioned 
>>> https://github.com/orocos/orocos_kinematics_dynamics/issues/41,
>>> problems where occuring in with SIP 4.13.2.
>>> 
>>> Which is explainable as you fixed this issue in the commit:
>>> https://www.riverbankcomputing.com/hg/sip/rev/0dd3cb4eff0e. This makes 
>>> sure
>>> the member variable contains a reference to the containing class. This
>>> prevents the class from being garbage collected while there are still
>>> references to the member variable.
>>> 
>>> You apply a fix to this in commit
>>> https://www.riverbankcomputing.com/hg/sip/rev/fbb9cdbad791.
>>> 
>>> The commit, 
>>> https://www.riverbankcomputing.com/hg/sip/rev/11a92ebd4840,
>>> which I also mentioned in my first message, is the important one.
>>> 
>>> In this commit you introduce a kind of caching for member variables. 
>>> The
>>> python wrapper of the members of a class are stored in a dict in the 
>>> python
>>> wrapper of the class. To add the member to the cache, you use
>>> "sipKeepReference(sipPySelf, %d, sipPy)". So the class contains a 
>>> reference
>>> to the member variable.
>>> 
>>> While previously "sipKeepReference(sipPy, -1, sipPySelf)", was used to 
>>> store
>>> the reference to the class in the dict of the member variable. Focus 
>>> here on
>>> the switch of "sipPy" and "sipPySelf". So by the last commit, 
>>> 11a92ebd4840,
>>> you added a new feature, but you broke another one.
>>> 
>>> I suggest to restore the feature which stores the reference to the 
>>> class in
>>> the member variable dict.
>>> 
>>> I did some testing in 4.19.20. I didn't experience a significant 
>>> performance
>>> increase with the caching with my Frame object and its member p, a 
>>> vector
>>> with 3 variables. (I tweaked the code,
>>> https://www.riverbankcomputing.com/hg/sip/file/7c54145e55b6/sipgen/gencode.c#l4904,
>>> to always get the caching.)
>>> Also I added manually the "sipKeepReference(sipPy, -1, sipPySelf)" to 
>>> the
>>> generated cpp for the class reference. When both features were used at 
>>> the
>>> same time, the "release" function of the class was not called. (I 
>>> added a
>>> "printf" to it, which didn't show in that situation, but did if just 
>>> of the
>>> two was used.)
>> 
>> Thanks for the analysis, that was very helpful.
>> 
>> This changeset is also a problem...
>> 
>> https://www.riverbankcomputing.com/hg/sip/rev/6ec87337d5e2
>> 
>> Can you try the latest version of the 4.19-maint branch, ie. including
>> this changeset...
>> 
>> https://www.riverbankcomputing.com/hg/sip/rev/137b9be794a1
>> 
>> ...and let me know how you get on.
> 
> There is now a newer changeset...
> 
> https://www.riverbankcomputing.com/hg/sip/rev/6a057b2d8537
> 
> Phil
> _______________________________________________
> PyQt mailing list    

> PyQt@

> https://www.riverbankcomputing.com/mailman/listinfo/pyqt

The first changeset already fixed the issue.

The newer changeset creates the situation as desribed in
http://python.6.x6.nabble.com/sip-Accessing-members-results-in-incorrect-missing-data-tp5266521p5266740.html,
it looks like the release function isn't called anymore. I thought Python GC
was able to break up circular references, but it looks like it isn't
happening.

By running the following code:
```
v = Vector(1,2,3)
R = Rotation()
F = Frame(R,v)
v2 = F.p
v3 = Frame(F).p
v3 = Vector(2,3,4)
```
The `release_Frame` is called(print shows up) after the second `v3`
assignment when using your first changeset, but the print doesn't show up
with your second changeset, today's one.

So to me it looks like this will cause a memory leak.

Side note: I would prefer the usage of a specific key for the member
reference to the containing class.
I don't think using the same key for both references will cause a problem
with overwriting. As a class can't contain a member of the same type, only a
member as a pointer to the same type. In which case, the reference isn't
kept, if I understand the code correct.

Matthijs



--
Sent from: http://python.6.x6.nabble.com/PyQt-f1792048.html
_______________________________________________
PyQt mailing list    [email protected]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt