Using particle field in move/copy constructors of MapNode

[tanya-kta]

Because it's not possible to allocate an array of MapNodes with the operator new[] when using an array of MapNodes user will face a problem: when trying to replace an element of the array with another MapNode object, the destructor will be called for the old object, which was never constructed (but instead was allocated with malloc or created in any other c-style way) and, therefore, has the particle field invalidated. When the destructor is called, it tries to delete a particle by the particle address, which is invalid. This results in an error.

The workaround is to call the detach_particle() method before replacing an array object, but it looks ugly and should be fixed on the MapNode class side

minds_crawl/src/simulation_objects/SimulationMap.cu

Line 246 in e150e17

nodes[n_of_nodes].detach_particle();

Also, the call to detach_particle() in the above piece of code must be commented. It looks absolutely out of place without a comment

[kolayne]

In fact, it turns out like MapNode is not responsible for its particle deletion (it only attaches already created particles and detaches them without deletion), so it's a bug that its destructor even tries to delete the particle!

[kolayne]

I thought about it a bit and have changed my mind. Even though MapNode is not responsible for its particle deletion, we expect MapNodes to only be deleted when the whole simulation is finished (i. e. when SimulationMap is being destructed). If this expectation is met, then the particle should be deleted because it won't be used anywhen again, and moreover, it should be deleted by MapNode because nobody else can do this (and unless a particle is deleted it's a memory leak).

Unfortunately, the application is not designed well enough: it pretends object-oriented, however, there is no one good interface for interaction with simulation (SimulationMap could/should have been this interface, but it is not): we have some functions in simulation_logic.cuh, which shouldn't be separate from the main class. It is still possible to move the particles' destruction to SimulationMap's area of responsibility, but I don't like this idea at all, because its current version does not interact with particles at all.

So, the perfect way I see is to change SimulationMap to make it Simulation with all the corresponding simulation logic functions' movements and their signatures' updates, but it's a rather big work and there is no time for it now.

[kolayne]

This should also be fixed for other classes. For example, we have a line

minds_crawl/src/main_logic/iterations_wrapper.cuh

Line 54 in e150e17

*simulation_map = SimulationMap(polyhedron);

which is likely to fail, because the destructor will be called for the invalid object

[kolayne]

The solution I see is adding default constructors for our classes. With them existing it would be possible to use the operator new instead If malloc, so the objects will be initialized with fake though destruction-safe values. This solution seems to work for most cases (including the one you have mentioned), however, CUDA does not allow using any memory but allocated with (c-style) cudaMalloc* in device functions (except for variables declared with __device__/__shared__ and variables created right in device code), so I don't think it's possible to run a constructor for an object which is intended to be used on a device but is created outside of a device/kernel function.
The simplest workaround I see for the case I've mentioned previously is running memset for the SimulationMap object before it's destructor will be executed, but this way will stop working if the destructor starts doing something more complicated than just freeing memory...

Another idea I came up with while writing the above is to create a friend function resetting an object to a safe-destructible instance. This solution requires an additional function call, but won't require a confusing default constructor to exist for classes. It also seems to work for all cases.

@tanya-kta, what do you think? Which of these three (default constructor + new, memset, friend function) do you consider the best for which case?

[kolayne]

I have chosen to add fake default constructors to the classes (replacing all the mallocs with news) and implementing resetting friend functions for the classes we can't avoid c-like allocation for (which currently are SimulationMap and Polyhedron). I prefer this over adding reset functions for all the classes to correctly handle special allocation cases (for example, if we don't know, what type we are allocating in a template function/class. It shouldn't be responsible for calling a reset function) and avoid accidental errors due to forgetting to call a reset function