Fixed OpenSim::Controller leaking memory whenever it is copied

[adamkewley]

This PR tries to fix a memory leak in OpenSim::Controller.

The leak occurs when copy-constructing or copy-assigning a Controller (or any derived class). It also happens when copying any class that contains a controller (e.g. an OpenSim::Model). When the conditions are triggered, Controller will leak copies of the OpenSim::Actuators it references. These can have fairly large memory footprints (e.g. because the Actuators are muscles containing paths, functions, etc.). A concrete example of a file that will do this is ToyLandingModel.osim (here), which contains a ToyReflexController. Each time ToyLandingModel.osim is copied, around ~1-2 MB of memory will leak.

The reason it happens is because of its _actuatorSet member. It is an OpenSim::Set<const Actuator> that, internally, has setMemoryOwner set to false. However, the original code doesn't account for the behavior of Set (more specifically, the OpenSim::ArrayPtrs it wraps), which is that--regardless of memory ownership--copy constructing (or assigning) a Set will unconditionally clone all of its elements, followed by re-setting setMemoryOwner to true. This causes problems because other parts of the code unconditionally call setMemoryOwner(false) later on, causing a leak.

This PR is a patch solution to the behavior. It adds user-defined copy constructor and copy assignment functions. These implementations try to implement the necessary copy semantics while avoiding the _actuatorSet.

A longer-term solution would be to use the sockets API to fix this. However, that may involve breaking the API, because the current controller API assumes that a set of Actuators is available via getActuators, setActuators, addActuator`, etc.

---

This change is 

[adamkewley]

@aseth1 if you have a second, could you review this? It can be a fairly big memory leak. Features like (e.g.) live plotting, background computing, etc. rely heavily on copying OpenSim::Models. Any workflows that want dynamic background computation are going to leak memory after every edit and, eventually, crash out.

The custom implementation of a copy ctor/assignment is necessary because the class is composing something that inherently leaks memory (explained in the original post).

[aymanhab]

LGTM other than minor refactoring to follow conventions elsewhere in the code for consistency. I opened issue #3275 a few weeks ago to get rid of this memoryOwner fiasco that we've been battling for a while. Would love to hear your feedback/comment there or if you want to take it on. Thank you

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @adamkewley)

---

OpenSim/Simulation/Control/Controller.h line 148 at r1 (raw file):

    // construct and initialize properties
    void constructProperties();

@adamkewley We use the convention of a separate function to constructProperties across the board, is there a strong reason to remove here and break consistency? If not I'd put it back.

---

OpenSim/Simulation/Control/Controller.cpp line 61 at r1 (raw file):

    ModelComponent{src},
    PropertyIndex_enabled{src.PropertyIndex_enabled},
    PropertyIndex_actuator_list{src.PropertyIndex_actuator_list},

PropertyIndex_ are never used in high level code, only for book-keeping inside base Component, would be good to keep this way (in case we ever change the macros or internals of Properties/Components).

[adamkewley]

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @aymanhab)

---

OpenSim/Simulation/Control/Controller.h line 148 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

@adamkewley We use the convention of a separate function to constructProperties across the board, is there a strong reason to remove here and break consistency? If not I'd put it back.

I can put it back - it's just that the helper method isn't used anywhere other than one location.

---

OpenSim/Simulation/Control/Controller.cpp line 61 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

PropertyIndex_ are never used in high level code, only for book-keeping inside base Component, would be good to keep this way (in case we ever change the macros or internals of Properties/Components).

I believe the reason why they are used is because the default-generated constructor would do the same (of recursively copy-constructing each member variable in the class). Is there an way to achieve 1:1 functionality with a default-implemented copy constructor? Or is the pattern to re-construct?

[adamkewley]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @aymanhab)

---

OpenSim/Simulation/Control/Controller.h line 148 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

I can put it back - it's just that the helper method isn't used anywhere other than one location.

It was put back

---

OpenSim/Simulation/Control/Controller.cpp line 61 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

I believe the reason why they are used is because the default-generated constructor would do the same (of recursively copy-constructing each member variable in the class). Is there an way to achieve 1:1 functionality with a default-implemented copy constructor? Or is the pattern to re-construct?

This will be kept as-is in the PR because it's fundamentally required to custom-implement the necessary copy constructor

[aymanhab]

@adamkewley modulo one comment that you can easily address

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamkewley)

---

OpenSim/Simulation/Control/Controller.cpp line 61 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

This will be kept as-is in the PR because it's fundamentally required to custom-implement the necessary copy constructor

@adamkewley My recollection is that these indices are populated by construction and so don't need to be "copied" from the src object, but if that's not the case then please ignore.

[adamkewley]

I opted to ignore the last comment @aymanhab - it's completely correct (initial construction of the object will also populate the indices, and those indices don't really change from instance to instance) but the default C++-generated copy constructors/assigners would also observe this behavior.

There's a chance (low, but who knows what future changes bring) that the indices may have somehow changed in the source object, from which the property table is being copied (via the base class copy constructor/assigner) and then there'd be an index mismatch at runtime.

Ideally, we'd all get some time to remove the setMemoryOwner APU, though, followed by nuking this patch!