Revert "[ML] Convert std::shared_ptrs to std::unique_ptrs where possible (#108)" (#114)

This reverts commit 71dc485.

Author: tveasey

docs/CHANGELOG.asciidoc

@@ -41,9 +41,6 @@ Secure the ML processes by preventing system calls such as fork and exec. The Li
 Seccomp BPF to intercept system calls and is available in kernels since 3.5. On Windows Job Objects prevent
 new processes being created and macOS uses the sandbox functionality ({pull}98[#98])
 
-Fix a bug causing us to under estimate the memory used by shared pointers and reduce the memory consumed
-by unnecessary reference counting ({pull}108[#108])
-
 === Bug Fixes
 
 Age seasonal components in proportion to the fraction of values with which they're updated ({pull}88[#88])

include/api/CForecastRunner.h

@@ -115,7 +115,7 @@ class API_EXPORT CForecastRunner final : private core::CNonCopyable {
     using TForecastModelWrapper = model::CForecastDataSink::SForecastModelWrapper;
     using TForecastResultSeries = model::CForecastDataSink::SForecastResultSeries;
     using TForecastResultSeriesVec = std::vector<TForecastResultSeries>;
-    using TMathsModelPtr = std::unique_ptr<maths::CModel>;
+    using TMathsModelPtr = std::shared_ptr<maths::CModel>;
 
     using TStrUSet = boost::unordered_set<std::string>;
 

include/core/CMemory.h

@@ -287,25 +287,21 @@ class CORE_EXPORT CMemory : private CNonInstantiatable {
     static std::size_t
     dynamicSize(const T& t,
                 typename boost::enable_if<typename boost::is_pointer<T>>::type* = nullptr) {
-        return t == nullptr ? 0 : staticSize(*t) + dynamicSize(*t);
+        if (t == nullptr) {
+            return 0;
+        }
+        return staticSize(*t) + dynamicSize(*t);
     }
 
     //! Overload for std::shared_ptr.
     template<typename T>
     static std::size_t dynamicSize(const std::shared_ptr<T>& t) {
-        if (t == nullptr) {
+        if (!t) {
             return 0;
         }
         long uc = t.use_count();
-        // Note we add on sizeof(long) here to account for the memory
-        // used by the shared reference count. Also, round up.
-        return (sizeof(long) + staticSize(*t) + dynamicSize(*t) + std::size_t(uc - 1)) / uc;
-    }
-
-    //! Overload for std::unique_ptr.
-    template<typename T>
-    static std::size_t dynamicSize(const std::unique_ptr<T>& t) {
-        return t == nullptr ? 0 : staticSize(*t) + dynamicSize(*t);
+        // Round up
+        return (staticSize(*t) + dynamicSize(*t) + std::size_t(uc - 1)) / uc;
     }
 
     //! Overload for boost::array.
@@ -696,40 +692,25 @@ class CORE_EXPORT CMemoryDebug : private CNonInstantiatable {
     static void dynamicSize(const char* name,
                             const std::shared_ptr<T>& t,
                             CMemoryUsage::TMemoryUsagePtr mem) {
-        if (t != nullptr) {
+        if (t) {
             long uc = t.use_count();
             // If the pointer is shared by multiple users, each one
             // might count it, so divide by the number of users.
             // However, if only 1 user has it, do a full debug.
             if (uc == 1) {
-                // Note we add on sizeof(long) here to account for
-                // the memory used by the shared reference count.
-                mem->addItem("shared_ptr", sizeof(long) + CMemory::staticSize(*t));
+                mem->addItem("shared_ptr", CMemory::staticSize(*t));
                 dynamicSize(name, *t, mem);
             } else {
                 std::ostringstream ss;
                 ss << "shared_ptr (x" << uc << ')';
-                // Note we add on sizeof(long) here to account for
-                // the memory used by the shared reference count.
-                // Also, round up.
-                mem->addItem(ss.str(), (sizeof(long) + CMemory::staticSize(*t) +
+                // Round up
+                mem->addItem(ss.str(), (CMemory::staticSize(*t) +
                                         CMemory::dynamicSize(*t) + std::size_t(uc - 1)) /
                                            uc);
             }
         }
     }
 
-    //! Overload for std::unique_ptr.
-    template<typename T>
-    static void dynamicSize(const char* name,
-                            const std::unique_ptr<T>& t,
-                            CMemoryUsage::TMemoryUsagePtr mem) {
-        if (t != nullptr) {
-            mem->addItem("ptr", CMemory::staticSize(*t));
-            memory_detail::SDebugMemoryDynamicSize<T>::dispatch(name, *t, mem);
-        }
-    }
-
     //! Overload for boost::array.
     template<typename T, std::size_t N>
     static void dynamicSize(const char* name,

include/maths/CChecksum.h

@@ -158,20 +158,13 @@ class CChecksumImpl<BasicChecksum> {
                        : CChecksumImpl<typename selector<T>::value>::dispatch(seed, *target);
     }
 
-    //! Checksum a shared pointer.
+    //! Checksum a pointer.
     template<typename T>
     static uint64_t dispatch(uint64_t seed, const std::shared_ptr<T>& target) {
         return !target ? seed
                        : CChecksumImpl<typename selector<T>::value>::dispatch(seed, *target);
     }
 
-    //! Checksum a unique pointer.
-    template<typename T>
-    static uint64_t dispatch(uint64_t seed, const std::unique_ptr<T>& target) {
-        return !target ? seed
-                       : CChecksumImpl<typename selector<T>::value>::dispatch(seed, *target);
-    }
-
     //! Checksum a pair.
     template<typename U, typename V>
     static uint64_t dispatch(uint64_t seed, const std::pair<U, V>& target) {

include/maths/CClusterer.h

@@ -125,6 +125,7 @@ class MATHS_EXPORT CClustererTypes {
 template<typename POINT>
 class CClusterer : public CClustererTypes {
 public:
+    using TClustererPtr = std::shared_ptr<CClusterer>;
     using TPointVec = std::vector<POINT>;
     using TPointPrecise = typename SPromoted<POINT>::Type;
     using TPointPreciseVec = std::vector<TPointPrecise>;

include/maths/CClustererStateSerialiser.h

@@ -43,7 +43,7 @@ struct SDistributionRestoreParams;
 //! to potential competitors.
 class MATHS_EXPORT CClustererStateSerialiser {
 public:
-    using TClusterer1dPtr = std::unique_ptr<CClusterer1d>;
+    using TClusterer1dPtr = std::shared_ptr<CClusterer1d>;
 
 public:
     //! Construct the appropriate CClusterer sub-class from its state
@@ -73,7 +73,7 @@ class MATHS_EXPORT CClustererStateSerialiser {
     //! \note Sets \p ptr to NULL on failure.
     template<typename T, std::size_t N>
     bool operator()(const SDistributionRestoreParams& params,
-                    std::unique_ptr<CClusterer<CVectorNx1<T, N>>>& ptr,
+                    std::shared_ptr<CClusterer<CVectorNx1<T, N>>>& ptr,
                     core::CStateRestoreTraverser& traverser) {
         return this->operator()(params, CClustererTypes::CDoNothing(),
                                 CClustererTypes::CDoNothing(), ptr, traverser);
@@ -87,7 +87,7 @@ class MATHS_EXPORT CClustererStateSerialiser {
     bool operator()(const SDistributionRestoreParams& params,
                     const CClustererTypes::TSplitFunc& splitFunc,
                     const CClustererTypes::TMergeFunc& mergeFunc,
-                    std::unique_ptr<CClusterer<CVectorNx1<T, N>>>& ptr,
+                    std::shared_ptr<CClusterer<CVectorNx1<T, N>>>& ptr,
                     core::CStateRestoreTraverser& traverser) {
         std::size_t numResults(0);
 

include/maths/CModelStateSerialiser.h

@@ -34,7 +34,7 @@ struct SModelRestoreParams;
 //! compatibility in the future as the classes evolve.
 class MATHS_EXPORT CModelStateSerialiser {
 public:
-    using TModelPtr = std::unique_ptr<CModel>;
+    using TModelPtr = std::shared_ptr<CModel>;
 
 public:
     //! Construct the appropriate CPrior sub-class from its state

include/maths/CMultimodalPrior.h

@@ -49,9 +49,11 @@ namespace maths {
 //! point of view this is the composite pattern.
 class MATHS_EXPORT CMultimodalPrior : public CPrior {
 public:
-    using TClustererPtr = std::unique_ptr<CClusterer1d>;
-    using TPriorPtr = std::unique_ptr<CPrior>;
+    using TClustererPtr = std::shared_ptr<CClusterer1d>;
+    using TPriorPtr = std::shared_ptr<CPrior>;
     using TPriorPtrVec = std::vector<TPriorPtr>;
+    using TPriorPtrVecItr = TPriorPtrVec::iterator;
+    using TPriorPtrVecCItr = TPriorPtrVec::const_iterator;
     using TMeanVarAccumulator = CBasicStatistics::SSampleMeanVar<double>::TAccumulator;
     using TMeanVarAccumulatorVec = std::vector<TMeanVarAccumulator>;
 
@@ -78,9 +80,6 @@ class MATHS_EXPORT CMultimodalPrior : public CPrior {
                      double decayRate = 0.0);
 
     //! Create from a collection of weights and priors.
-    //!
-    //! \note The priors are moved into place clearing the values in \p priors.
-    //! \note This constructor doesn't support subsequent update of the prior.
     CMultimodalPrior(maths_t::EDataType dataType, double decayRate, TPriorPtrVec& priors);
 
     //! Construct from part of a state document.
@@ -321,7 +320,7 @@ class MATHS_EXPORT CMultimodalPrior : public CPrior {
         CMultimodalPrior* m_Prior;
     };
 
-    using TMode = SMultimodalPriorMode<TPriorPtr>;
+    using TMode = SMultimodalPriorMode<std::shared_ptr<CPrior>>;
     using TModeVec = std::vector<TMode>;
 
 private:

include/maths/CMultimodalPriorMode.h

@@ -36,8 +36,6 @@ struct SMultimodalPriorMode {
     SMultimodalPriorMode() : s_Index(0), s_Prior() {}
     SMultimodalPriorMode(std::size_t index, const PRIOR_PTR& prior)
         : s_Index(index), s_Prior(prior->clone()) {}
-    SMultimodalPriorMode(std::size_t index, PRIOR_PTR&& prior)
-        : s_Index(index), s_Prior(std::move(prior)) {}
 
     //! Get the weight of this sample.
     double weight() const { return s_Prior->numberSamples(); }

include/maths/CMultivariateMultimodalPrior.h

@@ -32,7 +32,6 @@
 #include <maths/MathsTypes.h>
 
 #include <boost/bind.hpp>
-#include <boost/make_unique.hpp>
 #include <boost/numeric/conversion/bounds.hpp>
 #include <boost/ref.hpp>
 
@@ -51,10 +50,10 @@ namespace multivariate_multimodal_prior_detail {
 
 using TSizeDoublePr = std::pair<size_t, double>;
 using TSizeDoublePr3Vec = core::CSmallVector<TSizeDoublePr, 3>;
+using TPriorPtr = std::shared_ptr<CMultivariatePrior>;
 using TDouble10Vec1Vec = CMultivariatePrior::TDouble10Vec1Vec;
 using TDouble10VecWeightsAry1Vec = CMultivariatePrior::TDouble10VecWeightsAry1Vec;
-using TPriorPtr = std::unique_ptr<CMultivariatePrior>;
-using TMode = SMultimodalPriorMode<TPriorPtr>;
+using TMode = SMultimodalPriorMode<std::shared_ptr<CMultivariatePrior>>;
 using TModeVec = std::vector<TMode>;
 
 //! Implementation of a sample joint log marginal likelihood calculation.
@@ -135,7 +134,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
     using TMatrix = CSymmetricMatrixNxN<double, N>;
     using TMatrixVec = std::vector<TMatrix>;
     using TClusterer = CClusterer<TFloatPoint>;
-    using TClustererPtr = std::unique_ptr<TClusterer>;
+    using TClustererPtr = std::shared_ptr<TClusterer>;
     using TPriorPtrVec = std::vector<TPriorPtr>;
 
     // Lift all overloads of into scope.
@@ -163,13 +162,13 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
 
     //! Create from a collection of priors.
     //!
-    //! \note The priors are moved into place clearing the values in \p priors.
+    //! \note The priors are shallow copied.
     //! \note This constructor doesn't support subsequent update of the prior.
     CMultivariateMultimodalPrior(maths_t::EDataType dataType, TPriorPtrVec& priors)
         : CMultivariatePrior(dataType, 0.0) {
         m_Modes.reserve(priors.size());
         for (std::size_t i = 0u; i < priors.size(); ++i) {
-            m_Modes.emplace_back(i, std::move(priors[i]));
+            m_Modes.emplace_back(i, priors[i]);
         }
     }
 
@@ -426,12 +425,12 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
         for (const auto& mode : m_Modes) {
             TUnivariatePriorPtrDoublePr prior(mode.s_Prior->univariate(marginalize, condition));
             if (prior.first == nullptr) {
-                return {};
+                return TUnivariatePriorPtrDoublePr();
             }
             if (prior.first->isNonInformative()) {
                 continue;
             }
-            modes.push_back(std::move(prior.first));
+            modes.push_back(prior.first);
             weights.push_back(prior.second);
             maxWeight.add(weights.back());
         }
@@ -445,8 +444,8 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
             modes[i]->numberSamples(weights[i] / Z * modes[i]->numberSamples());
         }
 
-        return {boost::make_unique<CMultimodalPrior>(this->dataType(),
-                                                     this->decayRate(), modes),
+        return {TUnivariatePriorPtr(new CMultimodalPrior(this->dataType(),
+                                                         this->decayRate(), modes)),
                 Z > 0.0 ? maxWeight[0] + std::log(Z) : 0.0};
     }
 
@@ -466,7 +465,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
                                         const TSizeDoublePr10Vec& condition) const {
 
         if (N == 2) {
-            return {TPriorPtr(this->clone()), 0.0};
+            return TPriorPtrDoublePr(TPriorPtr(this->clone()), 0.0);
         }
 
         std::size_t n = m_Modes.size();
@@ -485,7 +484,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
             if (prior.first->isNonInformative()) {
                 continue;
             }
-            modes.push_back(std::move(prior.first));
+            modes.push_back(prior.first);
             weights.push_back(prior.second);
             maxWeight.add(weights.back());
         }
@@ -499,7 +498,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
             modes[i]->numberSamples(weights[i] / Z * modes[i]->numberSamples());
         }
 
-        return {boost::make_unique<CMultivariateMultimodalPrior<2>>(this->dataType(), modes),
+        return {TPriorPtr(new CMultivariateMultimodalPrior<2>(this->dataType(), modes)),
                 Z > 0.0 ? maxWeight[0] + std::log(Z) : 0.0};
     }
 
@@ -906,7 +905,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
 
             // Create the child modes.
             LOG_TRACE(<< "Creating mode with index " << leftSplitIndex);
-            modes.emplace_back(leftSplitIndex, TPriorPtr(m_Prior->m_SeedPrior->clone()));
+            modes.emplace_back(leftSplitIndex, m_Prior->m_SeedPrior);
             {
                 TPointVec samples;
                 if (!m_Prior->m_Clusterer->sample(
@@ -936,7 +935,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
             }
 
             LOG_TRACE(<< "Creating mode with index " << rightSplitIndex);
-            modes.emplace_back(rightSplitIndex, TPriorPtr(m_Prior->m_SeedPrior->clone()));
+            modes.emplace_back(rightSplitIndex, m_Prior->m_SeedPrior);
             {
                 TPointVec samples;
                 if (!m_Prior->m_Clusterer->sample(
@@ -1026,7 +1025,7 @@ class CMultivariateMultimodalPrior : public CMultivariatePrior {
                 MODE_TAG, TMode mode,
                 traverser.traverseSubLevel(boost::bind(
                     &TMode::acceptRestoreTraverser, &mode, boost::cref(params), _1)),
-                m_Modes.push_back(std::move(mode)))
+                m_Modes.push_back(mode))
             RESTORE_SETUP_TEARDOWN(
                 NUMBER_SAMPLES_TAG, double numberSamples,
                 core::CStringUtils::stringToType(traverser.value(), numberSamples),