How smart is Ptr?

[LTLA]

(Part of openjournals/joss-reviews#6617)

This section of the documentation suggests that Ptr is not smart, i.e., does not delete its contents upon destruction. I also had a look at the code and it seems that Ptr destruction triggers a decrement but not any deletion; an explicit call to Delete() is required.

This is fine if Ptr is intended as an exact replacement for a raw pointer (perhaps the docs could be more explicit about that). But it does seem like a missed opportunity for this library. Much of "modern C++" is moving towards smart pointers due to the ease of automatic memory management, and so it is unfortunate that emp::Ptr does not provide that.

On a related note, it appears that Ptr construction may not be thread-safe, due to the class-global PtrTracker that tracks all pointers. I don't see any locks to protect modifications for New(), etc., which may cause problems when used with OpenMP or by (Emscripten) PThreads. By comparison, std::shared_ptr operations are thread-safe.

[mercere99]

Thanks for these points -- they highlight some areas that we need to clarify in the documentation, as the need for thread safety when in debug mode with emp::Ptr.

The emp::Ptr class in Empirical is, indeed, not supposed to be smart in the sense of a managed pointer, though this is intentional. It is an "audited" poi
nter so that when you compile with debugging (in this case the -DEMP_TRACK_MEM flag), pointers are tracked and errors in their use are reported, but in release mode these checks are removed such that the Ptrs are reduced to raw pointers. If a coder wants to use smart pointers for automatic memory management, the std options are better choices for them and we didn't want to duplicate that functionality unless we were adding something meaningful to it.

Indeed, std::unique_ptr is always a good choice when it works for you, but there are some issues with std::shared_ptr that we are trying to get past. The lesser problem is that shared pointers carry overhead and require extra indirection when used. The associated slowdown is usually only felt in the most pointer-heavy sections of code, but on rare occasions can be substantial. The bigger problem that we've dealt with too many times is that std::shared_ptr encourages poor pointer hygiene as developers assume their pointers will be cleaned up when they are done with them, but often leave circular pointers floating off to nowhere with no warning coming from the system. Even for developers that are trying to be actively aware of this problem, it can be hard to avoid this type of memory leak.

The emp::Ptr class does a nice job of eliminating both of these problems while also preventing the most common problems from working with raw pointers. It does expect the users to manually run Delete() on pointers when done with them, but will report any issues of pointers that are either used inappropriately (such as after being deleted) or not cleaned up properly. It also provides tools to track pointers over their lifetimes to help with debugging.

All of that being said, you are right that the current implementation is not thread safe when in debug mode, and that's something that we need to fix. And, of course, the points above need to be clarified in the documentation.

Thanks again -- we appreciate the feedback.

[LTLA]

Makes sense to me. Feel free to close, unless you want to keep it open to track the thread safety issue.

[mmore500]

Closing this and opening an issue specifically on the thread safe issue 👍