QMCPACK / QMCPACK/qmcpack

Need smart pointers. Using LCAOrbitalSetWithCorrection as a starting example

Open
#1,624 7 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
C++
Stars
403
Forks
154
Avg merge
1d 12h
Merged PRs (30d)
82

Description

#1623 exposed a problem that we need to reduce the use of raw pointers and use smart pointers if pointers are needed to share data between class objects.

I extract lines of code to better describe a right way to do in my mind.
```
struct LCAOrbitalSetWithCorrection : public LCAOrbitalSet
{
SoaCuspCorrection cusp;

SPOSet* LCAOrbitalSetWithCorrection::makeClone() const
{
LCAOrbitalSetWithCorrection* myclone = new LCAOrbitalSetWithCorrection(*this);
myclone->myBasisSet = myBasisSet->makeClone();
myclone->IsCloned = true;
return myclone;
}
}

struct SoaCuspCorrection
{
typedef CuspCorrectionAtomicBasis COT;
aligned_vector LOBasisSet; // raw pointer, bad

SoaCuspCorrection(const SoaCuspCorrection& a) = default;
}

template
class CuspCorrectionAtomicBasis
{
typedef MultiQuinticSpline1D RadialSetType;
RadialSetType AOs;
std::vector phi, dphi, d2phi;

CuspCorrectionAtomicBasis(const CuspCorrectionAtomicBasis& a) = default;

inline void evaluate(const T r, T* restrict vals)
{
phi.resize(nr);
}
}

template
class MultiQuinticSpline1D
{
coeff_type* Coeffs; // raw pointer, bad
bool own_spline;

MultiQuinticSpline1D(const MultiQuinticSpline1D& in) = default;

MultiQuinticSpline1D* makeClone() const
{
MultiQuinticSpline1D* myclone = new MultiQuinticSpline1D(*this);
myclone->own_spline = false;
return myclone;
}
}
```

Each thread has a copy of `LCAOrbitalSetWithCorrection`. The makeClone function triggers the copy constructor and copies the pointers of `SoaCuspCorrection::LOBasisSet` by value instead of deep copy. Multple threads attempt to resize in `CuspCorrectionAtomicBasis::evaluate` and breaks the code.

The code was correct before #1611 by defining phi[nr] local to the function. This is dangerous due to VLA and may have low performance due to allocation and deallocation on the fly per thread. For the moment, the VLA is replaced with std::vector phi(nr) defined local to the function but the allocation and deallocation issue remains. So this is a workaround.

The root cause of this problem is that `SoaCuspCorrection` uses the default copy constructor but it was not made safe. See `MultiQuinticSpline1D` an example, when there is a raw pointer in the structure, a specialized copy function (`MultiQuinticSpline1D::makeClone`) is needed to manage the ownership of the pointer. This is actually a misuse of makeClone. makeClone is only needed when there are base and derived classes. Instead, we should use C++ smart pointers and use shared_ptr for Coeffs. In this way, the compiler will handle the ownership ofCoeffs and the class can be made destruction safe. After using shared pointer, the makeClone is no more needed in MultiQuinticSpline1D and the default copy constructor of MultiQuinticSpline1D and CuspCorrectionAtomicBasis are safe to use.

In SoaCuspCorrection, LOBasisSet should be replaced as `aligned_vector` without pointers. In this way, SoaCuspCorrection has a safe default copy constructor.

There is a similar situation in `myBasisSet` that we can make better use of smart pointer and thus IsCloned state doesn't need to be explicitly managed and we can safely destructor SoaCuspCorrection class. The desired code should have LCAOrbitalSetWithCorrection default copy constructor safe and makeClone function should only contains a line invoking the copy constructor.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.