Only call loadPlugins once in constructors.#469
Conversation
libsbml is leaking memory in the loadPlugins functions, and my working theory is that this is because some constructors call it multiple times: once in the base class, then again in the derived class. This change removes *all* calls of loadPlugins for all derived classes where the base class also calls loadPlugins. I'm not sure why we don't call loadPlugins in the SBase constructor directly, but changing that would be a fairly significant design change, so no need to try it now. I believe that we cannot do the same thing with 'connectToChild', because it becomes important when it's the derived class that overrides the base definition for it, and I *think* that a base class constructor would not call the derived class version. If I'm wrong, we can revisit, but it's also true that connectToChild doesn't leak memory, so it's not dangerous to call it multiple times. 'setElementNamespace' also only needs to be called once, but this is sort of incidental to the other cleanups. I have not yet tested whether this change actually fixes the leak--needed to check this in from my development machine so I could check it out on my valgrind-enabled machine.
|
OK, I went through the changes, fixed a bug, and I think it's ready to go. There are probably similar fixes that could be made in other packages, but this is good for now! The basic idea is that we don't need to call setElementNamespace or loadPlugins in constructors where those functions are already called in their base classes, but you do still want to call connectToChild(), since that functions on sub-class elements. |
(Actually mostly an excuse to get a new push in, so the test-on-push works again.)
Pull develop into branch
|
OK, this is working! @fbergmann : This is not a critical update, but it would be nice. It calls functions less frequently, and is more robust about calling 'connectToChild'. |
fbergmann
left a comment
fbergmann
left a comment
There was a problem hiding this comment.
works for me, but for distrib ...w e are now calling connectToChild in distribbase (even though there is a loadplugins call as well, and while we are at it ... if calling connectToChild there, should it not be called after loading the plugins?
been too long that i looked into this !
|
connectToChild sets the parent object of all the object's children, but not plugins. So I believe that you can call it before or after loading any plugins; those objects' parents are set separately? |
2 participants
I have memory leaks from loadPlugins functions from somewhere, and my working theory was that this is because some constructors call it multiple times: once in the base class, then again in the derived class. This change removes all calls of loadPlugins for all derived classes where the base class also calls loadPlugins.
I'm not sure why we don't call loadPlugins in the SBase constructor directly, but changing that would be a fairly significant design change, so no need to try it now.
I believe that we cannot do the same thing with 'connectToChild', because it becomes important when it's the derived class that overrides the base definition for it, and I think that a base class constructor would not call the derived class version. If I'm wrong, we can revisit, but it's also true that connectToChild doesn't leak memory, so it's not dangerous to call it multiple times.
'setElementNamespace' also only needs to be called once, but this is sort of incidental to the other cleanups.
The change didn't end up fixing the leak, but it's still more efficient.