Fix #868: Recoups some of performance losses from PR #841 (#869) (#907)

* fixed lockReg shared_ptr * down to 5% increase from original * get logs from github to verify behavior * reverting performance yml and adding move constructor (default) for RegistrationLocks : Co-authored-by: tcormackMW <113473781+tcormackMW@users.noreply.github.com>
CppMicroServices · Aug 29, 2023 · f6d070e · f6d070e · github-actions · Aug 29, 2023
1 parent f331e12
commit f6d070e
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 23 deletions.
diff --git a/framework/include/cppmicroservices/ServiceRegistrationBase.h b/framework/include/cppmicroservices/ServiceRegistrationBase.h
@@ -201,7 +201,7 @@ namespace cppmicroservices
 
         ServiceRegistrationBase(BundlePrivate* bundle, InterfaceMapConstPtr const& service, Properties&& props);
 
-        std::shared_ptr<ServiceRegistrationLocks> LockServiceRegistration() const;
+        ServiceRegistrationLocks LockServiceRegistration() const;
 
         std::shared_ptr<ServiceRegistrationBasePrivate> d;
     };

diff --git a/framework/src/service/ServiceReferenceBasePrivate.cpp b/framework/src/service/ServiceReferenceBasePrivate.cpp
@@ -55,10 +55,10 @@ namespace cppmicroservices
 
     ServiceReferenceBasePrivate::~ServiceReferenceBasePrivate() = default;
 
-    std::shared_ptr<ServiceRegistrationLocks>
+    ServiceRegistrationLocks
     ServiceReferenceBasePrivate::LockServiceRegistration() const
     {
-        return std::make_shared<ServiceRegistrationLocks>(registration.lock(), coreInfo);
+        return ServiceRegistrationLocks(registration.lock(), coreInfo);
     }
 
     InterfaceMapConstPtr

diff --git a/framework/src/service/ServiceReferenceBasePrivate.h b/framework/src/service/ServiceReferenceBasePrivate.h
@@ -55,7 +55,7 @@ namespace cppmicroservices
 
         ~ServiceReferenceBasePrivate();
 
-        std::shared_ptr<ServiceRegistrationLocks> LockServiceRegistration() const;
+        ServiceRegistrationLocks LockServiceRegistration() const;
 
         /**
          * Get the service object.

diff --git a/framework/src/service/ServiceRegistrationBase.cpp b/framework/src/service/ServiceRegistrationBase.cpp
@@ -330,10 +330,10 @@ namespace cppmicroservices
         }
     }
 
-    std::shared_ptr<ServiceRegistrationLocks>
+    ServiceRegistrationLocks
     ServiceRegistrationBase::LockServiceRegistration() const
     {
-        return std::make_shared<ServiceRegistrationLocks>(d, d->coreInfo);
+        return ServiceRegistrationLocks(d, d->coreInfo);
     }
 
     bool
@@ -353,12 +353,9 @@ namespace cppmicroservices
             return true;
         }
 
-        ServiceReferenceBase sr1;
-        ServiceReferenceBase sr2;
-        {
-            LockServiceRegistration(), sr1 = d->reference;
-            o.LockServiceRegistration(), sr2 = o.d->reference;
-        }
+        ServiceReferenceBase sr1 = (LockServiceRegistration(), d->reference);
+        ServiceReferenceBase sr2 = (o.LockServiceRegistration(), o.d->reference);
+
         return sr1 < sr2;
     }
 

diff --git a/framework/src/util/ServiceRegistrationLocks.cpp b/framework/src/util/ServiceRegistrationLocks.cpp
@@ -24,14 +24,11 @@
 namespace cppmicroservices
 {
 
-    ServiceRegistrationLocks::ServiceRegistrationLocks(std::shared_ptr<ServiceRegistrationBasePrivate> reg,
-                                                       std::shared_ptr<ServiceRegistrationCoreInfo> coreInfo)
-    {
-        if (reg != nullptr)
-        {
-            regL = reg->Lock();
-        }
-        coreInfoL = coreInfo->Lock();
+    ServiceRegistrationLocks::ServiceRegistrationLocks(const std::shared_ptr<ServiceRegistrationBasePrivate>& reg,
+                                                       const std::shared_ptr<ServiceRegistrationCoreInfo>& coreInfo)
+        : regL(reg != nullptr ? reg->Lock() : cppmicroservices::detail::MutexLockingStrategy<>::UniqueLock())
+        , coreInfoL(coreInfo->Lock())
+    {      
     }
 } // namespace cppmicroservices
 

diff --git a/framework/src/util/ServiceRegistrationLocks.h b/framework/src/util/ServiceRegistrationLocks.h
@@ -35,12 +35,18 @@ namespace cppmicroservices
     class ServiceRegistrationLocks final
     {
       public:
-        ServiceRegistrationLocks(std::shared_ptr<ServiceRegistrationBasePrivate> reg,
-                                 std::shared_ptr<ServiceRegistrationCoreInfo> coreInfo);
+        ServiceRegistrationLocks(const std::shared_ptr<ServiceRegistrationBasePrivate>& reg,
+                                 const std::shared_ptr<ServiceRegistrationCoreInfo>& coreInfo);
+
+        // Delete all copy and move to enforce that it is only ever constructed into one object -- avoids deadlocks
+        ServiceRegistrationLocks(ServiceRegistrationLocks const& lockObj) = delete;
+        ServiceRegistrationLocks(ServiceRegistrationLocks&& lockObj) = default;
+        ServiceRegistrationLocks& operator=(ServiceRegistrationLocks const& lockObj) = delete;
+        ServiceRegistrationLocks& operator=(ServiceRegistrationLocks&& lockObj) = delete;
 
       private:
-        cppmicroservices::detail::MutexLockingStrategy<>::UniqueLock coreInfoL;
         cppmicroservices::detail::MutexLockingStrategy<>::UniqueLock regL;
+        cppmicroservices::detail::MutexLockingStrategy<>::UniqueLock coreInfoL;
     };
 } // namespace cppmicroservices