Don't store pointers in the ABM

[dabele]

Motivation / Current Behaviour

Storing pointers to persons and locations in the ABM is convenient for programming, since it reduces the number of function arguments. But storing indices and only passing the actual objects when needed is much more flexible. Apart from performance improvements, it would also enable e.g. copying the World.

Two major performance problems that are caused by storing pointers:

• Adding and removing persons during migration is expensive and requires a mutex for multithreading
• iterating is slower because of reduced locality.

Enhancement description

Persons don't store a pointer to locations. Instead they store an index, the location object is passed by the World as a function argument when necessary.

Locations don't store persons at all. When some accumulation of persons per Location is necessary, this should be done by iterating over all persons and caching the result. This requires less synchronization. (It might become necessary later for the locations to store indices to its persons. This can be added later.)

The world can store the locations and persons directly and doesn't need unique_ptr.

Additional context

No response

Checklist

• Attached labels, especially loc:: or model:: labels.
• Linked to project

[khoanguyen-dev]

I also add some comments from David's:

Perhaps we should think about out-sourcing the mutex and the vector of persons to an extra struct, so that we can avoid writing any SMF for the Location? Also, the vector of Persons should probably be locked for the copy-duration, although this is not really a use-case for use, but just to make sure.

And Daniel's:

Ultimately, the solution is to not store the persons and avoid the mutex. But that's a different discussion we need to have at some point.

I agree you would probably want the mutex in a wrapper so you only need to write explicit copy constructor for one thing instead of all the data members of Location. Easy to forget something.

If you want to lock the mutex while copying, it needs to be declared mutable. I don't really care, since copying locations or anything else while the simulation is running is completely invalid and I don't see why someone would do it except accidentally, in which case a crash is a good thing.

(In fact I kind of liked non copyable Locations since I detected a number of accidental copies that cost a lot of performance. From a safety point of view, I would prefer locations and persons not to have a copy constructor, but some explicit copy function, but I see how that would be too difficult to maintain without the help of default copy constructors. I think all we can do is watch out for accidental copies like auto loc = person.get_location())