Only call loadPlugins once in constructors. #469
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Member
Author
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The change didn't end up fixing the leak, but it's still more efficient.