Smart pointers

[pazner]

There has been an ongoing discussion on further incorporating smart pointers (in particular, shared/reference-counted pointers) in MFEM. In order to be backwards compatible, it seems required to model "maybe owning" semantics — the smart pointer may own the pointed-to object, or it may not, and this depends on runtime information. Perhaps the simplest example is something like ProductOperator, whose declaration is given here:

mfem/linalg/operator.hpp

Lines 892 to 909 in 899a96b

	/// General product operator: x -> (A*B)(x) = A(B(x)).
	class ProductOperator : public Operator
	{
	const Operator *A, *B;
	bool ownA, ownB;
	mutable Vector z;
	public:
	ProductOperator(const Operator *A, const Operator *B, bool ownA, bool ownB);
	void Mult(const Vector &x, Vector &y) const override
	{ B->Mult(x, z); A->Mult(z, y); }
	void MultTranspose(const Vector &x, Vector &y) const override
	{ A->MultTranspose(x, z); B->MultTranspose(z, y); }
	virtual ~ProductOperator();
	};

Depending on the boolean flags ownA and ownB, the resulting ProductOperator may or may not own the pointed-to operators. The current implementation has incorrect copy/move semantics, which may cause issues in otherwise well-formed code (e.g. storing ProductOperator in a standard container), and the manual management of ownership rules may cause subtle bugs/memory leaks (see e.g. #4613 from as recently as today).

We can modify ProductOperator (and similar classes) in a fully backwards-compatible way using smart pointers that model "maybe owning" semantics. We have discussed two approaches to achieve this:

1. Using std::shared_ptr. An "owning" shared_ptr is created as usual, whereas a "non-owning" shared_ptr is created with a no-op deleter, e.g. [](T*){}. Convenience functions can be used to make specifying the ownership convenient and explicit.
   • Advantage: this uses standard classes and the implementation is correct. It is fully compatible with other code that uses shared_ptr.
   • Disadvantage: the non-owning version is a somewhat non-standard use of shared_ptr (it was referred to in the meeting as an "abuse") and it may confuse users. We can't provide our own documentation for this class. An existing shared_ptr can't be easily queried about whether it is owning or not.
2. Using a custom type (e.g. Handle<T>) that is built on top of shared_ptr.
   • Advantage: we can fully document the semantics of this type, users won't be surprised by "non-owning" pointers, we can add functions to query whether it is owning or not, and we can potentially add extra functionality if needed to be applicable to more classes in MFEM.
   • Disadvantage: a custom type will not be familiar to users, and careful implementation will be required to avoid bugs.

[v-dobrev]

Note that the no-op deleter can be the same for all types T -- the argument can be just void* -- the only requirement for the deleter is that the expression deleter(p) makes sense when p is T*.

[termi-official]

In order to be backwards compatible, it seems required to model "maybe owning" semantics — the smart pointer may own the pointed-to object, or it may not, and this depends on runtime information.

Maybe a wild take on this, but don't you think it is worth breaking backwards compat in turn to simplify lifetime management? This should be done with a clear upgrade path described for users which want to go forward. From my understanding the "maybe owning" semantics stems from the issue that the objects are not reference counted in MFEM, hence it is not a-priori clear which object has to live for how long a priori. Hence there are flags introduced to manage this lifetime information manually. Keeping the product operator you have at top as an example, with shared pointer this problem would not occur, as the pointer itself manages its lifetime (by reference counting).

Furthermore, for non-owning semantics we have https://en.cppreference.com/w/cpp/memory/weak_ptr

[rcarson3]

@v-dobrev and @pazner circling back to this topic, if we had a deprecation period then it seems like option 1 above would be the best medium term solution as we could "easily" support both the smart pointer and raw pointer APIs over a set of releases before deprecating the old API. During this time, we can point users to the new API using something like [[deprecated("Use constructor with std::shared_ptr<T> instead")]] Afterwards, you could remove the raw pointers usage and now have smart pointers in their place. Although, I would note that for the above I'm thinking more of like the class member variables (aka the mesh, FES, QSpace, etc...) that aren't things like the raw data which are typically behind the Memory object.

I'm bringing this up as I've been looking at implementing PartialQuadratureSpace and PartialQuadratureFunction extensions (both derived from QuadratureSpaceBase and QuadratureFunction) where I'm trying to leverage smart pointers where it makes sense. I'm still in the middle of this work, but I'm finding that going with Option 1 and adding that to my branch of MFEM for QuadratureSpaceBase (and its derived classes) and QuadratureFunction to be the one with the least friction from an end user point of view. Since, I can easily transition my code base and make use of standard smart pointers.

[rcarson3]

@v-dobrev and @pazner for example this is how I'm starting to tackle it in one of my branches and outside of the warnings it just works with existing code or at least what I've tested so far:

rcarson3@d791160

A few minor changes are needed to some constructors to reduce number of changes for the QSpaceBase and FaceQSpace constructor use cases. I'd be very much open to discussing this more and if there might be ways forward to slowly introduce smart pointers into mfem.

Also looks like some of my other small c++17 feature additions crept into that commit but overall the commit is focused on the smart pointer changes.

[pazner]

Thanks @rcarson3, this is useful.

For reference, I have a WIP PR implementing option 2 here: #4629. I think it is largely compatible with your approach: you can convert back-and-forth between std::shared_ptr and MFEM's "handle". Feel free to take a look and let me know if you have any thoughts/suggestions.

[tepperly]

If you're going through the work of switching to smart pointers, it would be good to reap the benefits and not have things that behave partially right.

In the case of a std::shared_ptr that's referring to data that's not actually owned by the smart pointer, what guarantee is that the reference object will keep living as long as the program holding the std::shared_ptr keeps the reference? In the situation where the data isn't owned, the referenced object could disappear without the shared_ptr holders released their hold, right? That seems like a major violation of expectations.

[pazner]

There is not a guarantee, only a "promise". The contract of the API is that if the user passes a non-owning Handle to another object, then the pointed-to object must have a longer lifetime.

Keep in mind that in C++, neither shared_ptr nor unique_ptr offer hard guarantees either. (They both can be null, and use-after-free, double-free, etc. are all possible).

[tepperly]

This is a follow up to my brief question on the slack #helpdesk channel.

My opinion is that before tackling shared_ptr, you should utilize ::std::unique_ptr in every situation where MFEM already asserts ownership over data. For example, various flavors of the AddDomainIntegrator methods take a raw pointer that MFEM unambiguously takes ownership of. Adding a method that takes a std::unique_ptr instead of a raw pointer that must have been allocated with new makes this ownership clear, explicit, and compiler enforced. It eliminates the problem of forgetting the delete or skipping the delete due to an exception.

Similarly, some methods like ParGridFunction::GetTrueDofs() unambiguously transfer ownership of the returned pointer to the caller. This is another situation where std::unique_ptr<HypreParVector> makes this semantics clear. If the caller ignores the return value for some reason, the allocated pointer gets cleaned up. If the caller needs to share the return value, one can convert a ::std::unique_ptr into a std::shared_ptr.

    ::std::unique_ptr<HypreParVector> GetTrueDofsU() const;

You could gradually deprecate the raw pointer versions of those methods.

The decision about how to implement shared pointers is difficult because there are various costs and benefits. The great thing about std::unique_ptr is that there are no runtime costs (memory or CPU). There is some additional typing involved, but you do have some keystrokes from not having to have a delete statement.

IMHO, you should also modify all the examples and miniapps to use unique_ptr's rather than new/delete. Programming by cut-n-paste is ubiquitous, so you examples should utilize best practices to encourage best practices in the community.

[pazner]

Overall I think something like this would be a step in the right direction. In situations where ownership is unambiguously unique, we should use unique_ptr. To transfer ownership, we should return unique_ptr (and maybe also pass unique_ptr by value as you suggest). Often even better than returning unique_ptr would be to return by value in cases where the object is movable and/or we can be guaranteed that any copy will be elided.

In most of the examples, the new and delete just by using RAII rather than smart pointers.

[tepperly]

Is there an approach 3 where you introduce a new set of methods where clean std::unique_ptr and std::shared_ptr can be used with their normal semantics. The maybe owned semantics are maintained in a legacy API that's eventually deprecated. New features are implemented with the clean smart pointer API. Perhaps, some of the cases in the maybe owned semantics API can be implemented via the normal smart pointers. Approaches 1 and 2 above both seem regrettable to me.

[v-dobrev]

I'm not sure I understand the difference between approach 1 and what you call approach 3. Is it because of the "maybe owned" usage of shared_ptr?

The "maybe owned" semantic for shared_ptr is part of the C++ standard (allowing custom deleters), however, we don't see this as something users will need to worry about, i.e. we don't expect users to create such shared_ptr objects to pass to MFEM. It is just a way for MFEM to implement the current behavior of some methods that use pure pointers (where the pointer can be either owned or not).

[tepperly]

The semantics of shared_ptr is that the referenced object will remain as long as someone holds a reference to it. For example, I should be able to use a shared_ptr in a function registered with atexit() that runs after main() ends.

[v-dobrev]

The "not owning" shared_ptr will be used only when "legacy" methods are called with pure pointers and ownership set to false -- this is to ensure backward compatibility until these "legacy" methods are removed.

[pazner]

shared_ptr (without non-owning custom deleters) in user-facing APIs is a bit of a double-edged sword. It forces the user code to either release ownership or also use shared_ptr. Because of this, it can propagate through the code very significantly.

As an example, GridFunction needs to be constructed with a FiniteElementSpace, and the lifetime of the space needs to be at least as long as the lifetime of the grid function. The same space can be shared between multiple grid functions. So it could be natural to require that GridFunction is constructed with shared_ptr<FiniteElementSpace>. But this then implies that, in order to do anything useful with a finite element space, it needs to first be wrapped in a shared_ptr. The same is true for most of the other classes you might use in MFEM; the space itself is constructed with a Mesh, and the mesh can be shared between multiple spaces and must outlive the space, so we need shared_ptr<Mesh>. Then, everything becomes a tangle of shared_ptr. This is a feasible design choice, but I don't think there is a consensus around a change like this, and the (over) use of shared_ptr comes with its own set of drawbacks.

The idea with the "maybe owning semantics" is that the user has the option of taking responsibility for the lifetime of the object, and they promise that they will ensure that it lives at least as long as is required. Obviously this is more dangerous (you don't get the same guarantees that you do with reference counting), but it also makes the API much less restrictive.

[justinlaughlin]

With mfem 5.0 on the horizon, should we have some technical discussions on how we want to move forward with this? I think there is consensus that using unique_ptr where ownership is unambiguous is a good idea, but there is less consensus on how to handle shared ownership. Based on the poll a custom "maybe owning pointer" seems preferred over shared_ptr/weak_ptr [like @pazner 's demo].

I've been discussing with @dylan-copeland , @IdoAkkerman , and @helloworld922 about this in the context of nurbs; for that specific problem every solution feels like choosing between degrees of lesser-evils, but the root issue comes down to unclear ownership semantics. It would be very nice to have clarity on the MFEM-preferred solution, rather than me implementing my own idea (and someone else implementing their own, etc.).

[justinlaughlin]

Tagging @tzanio and @v-dobrev for some extra decision-making momentum :)

[rcarson3]

Putting my MFEM application hat on, I'd also be very interested in this discussion as well as I'd very much like to move my code over to smart pointers from the STL and not have to rely on a branch of MFEM that supports that. I will also say that at least from my own branch that is leveraging shared_ptrs in the QuadratureFunctions \ Spaces (am thinking of eventually expanding that to FES and other objects next) that it made dealing with ownership a lot easier and I'm no longer just handing out raw pointers everywhere with no guarantees on lifetime of objects.

Long story short, I'm a big fan of any movement in the right direction from raw pointers used everywhere...

[justinlaughlin]

Long story short, I'm a big fan of any movement in the right direction from raw pointers used everywhere...

Agreed! I don't have a strong opinion on the specific solution for ownership semantics but I would really like to see us commit to something and go with it.