Improve performance of LDAP matching (#704)

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>
CppMicroServices · Aug 1, 2022 · 27e035d · 27e035d
1 parent 9a342dc
commit 27e035d
Show file tree

Hide file tree

Showing 15 changed files with 576 additions and 107 deletions.
diff --git a/framework/include/cppmicroservices/AnyMap.h b/framework/include/cppmicroservices/AnyMap.h
@@ -29,6 +29,8 @@
 #include <unordered_map>
 
 namespace cppmicroservices {
+class Properties;
+class LDAPExpr;
 
 namespace detail {
 
@@ -111,7 +113,8 @@ class US_Framework_EXPORT any_map
 
     iterator_base(iter_type type)
       : type(type)
-    {}
+    {
+    }
 
   public:
     using value_type = any_map::value_type;
@@ -221,8 +224,11 @@ class US_Framework_EXPORT any_map
 
   any_map(map_type type);
   any_map(const ordered_any_map& m);
+  any_map(ordered_any_map&& m);
   any_map(const unordered_any_map& m);
+  any_map(unordered_any_map&& m);
   any_map(const unordered_any_cimap& m);
+  any_map(unordered_any_cimap&& m);
 
   any_map(const any_map& m);
   any_map& operator=(const any_map& m);
@@ -297,6 +303,30 @@ class US_Framework_EXPORT any_map
   map_type type;
 
 private:
+  friend class Properties;
+  friend class LDAPExpr;
+
+  // Private "fast" and type-checked functions for working with map iterators
+  // and finding elements (these functions should only be called in a context)
+  // where the type of the map is guaranteed.
+  //
+  // These functions bypass the creation of "any_map::iterator" and "any_map::const_iterator"
+  // objects as construction of those are slow. Once the AnyMap class is refactored, these
+  // functions will likely be unnecessary as the begin(), end(), and find() functions will
+  // inherently do what these functions do.
+  ordered_any_map::const_iterator beginOM_TypeChecked() const;
+  ordered_any_map::const_iterator endOM_TypeChecked() const;
+  ordered_any_map::const_iterator findOM_TypeChecked(const key_type& key) const;
+  unordered_any_map::const_iterator beginUO_TypeChecked() const;
+  unordered_any_map::const_iterator endUO_TypeChecked() const;
+  unordered_any_map::const_iterator findUO_TypeChecked(
+    const key_type& key) const;
+  unordered_any_cimap::const_iterator beginUOCI_TypeChecked() const;
+  unordered_any_cimap::const_iterator endUOCI_TypeChecked() const;
+  unordered_any_cimap::const_iterator findUOCI_TypeChecked(
+    const key_type& key) const;
+  // =========================================================================
+
   ordered_any_map const& o_m() const;
   ordered_any_map& o_m();
   unordered_any_map const& uo_m() const;

diff --git a/framework/src/CMakeLists.txt b/framework/src/CMakeLists.txt
@@ -14,6 +14,7 @@ set(_srcs
   util/LDAPFilter.cpp
   util/LDAPProp.cpp
   util/Properties.cpp
+  util/PropsCheck.cpp
   util/SecurityException.cpp
   util/SharedLibrary.cpp
   util/SharedLibraryException.cpp
@@ -65,6 +66,7 @@ set(_private_headers
   util/CFRLogger.h
   util/LDAPExpr.h
   util/Properties.h
+  util/PropsCheck.h
   util/Utils.h
 
   service/ServiceHooks.h
@@ -90,4 +92,3 @@ set(_private_headers
   bundle/CoreBundleContext.h
   bundle/Resolver.h
 )
-
diff --git a/framework/src/service/ServiceListeners.cpp b/framework/src/service/ServiceListeners.cpp
@@ -451,13 +451,13 @@ void ServiceListeners::GetMatchingServiceListeners(const ServiceEvent& evt,
 
     // Check the cache
     const auto c = any_cast<std::vector<std::string>>(
-      props->Value_unlocked(Constants::OBJECTCLASS));
+      props->Value_unlocked(Constants::OBJECTCLASS).first);
     for (auto& objClass : c) {
       AddToSet_unlocked(set, receivers, OBJECTCLASS_IX, objClass);
     }
 
     auto service_id =
-      any_cast<long>(props->Value_unlocked(Constants::SERVICE_ID));
+      any_cast<long>(props->Value_unlocked(Constants::SERVICE_ID).first);
     AddToSet_unlocked(set,
                       receivers,
                       SERVICE_ID_IX,

diff --git a/framework/src/service/ServiceReferenceBase.cpp b/framework/src/service/ServiceReferenceBase.cpp
@@ -80,7 +80,7 @@ Any ServiceReferenceBase::GetProperty(const std::string& key) const
 {
   auto l = d.load()->registration->properties.Lock();
   US_UNUSED(l);
-  return d.load()->registration->properties.Value_unlocked(key);
+  return d.load()->registration->properties.Value_unlocked(key).first;
 }
 
 void ServiceReferenceBase::GetPropertyKeys(std::vector<std::string>& keys) const
@@ -144,11 +144,14 @@ bool ServiceReferenceBase::operator<(
   {
     auto l = d.load()->registration->properties.Lock();
     US_UNUSED(l);
-    anyR1 = d.load()->registration->properties.Value_unlocked(
-      Constants::SERVICE_RANKING);
+    anyR1 =
+      d.load()
+        ->registration->properties.Value_unlocked(Constants::SERVICE_RANKING)
+        .first;
     assert(anyR1.Empty() || anyR1.Type() == typeid(int));
-    anyId1 =
-      d.load()->registration->properties.Value_unlocked(Constants::SERVICE_ID);
+    anyId1 = d.load()
+               ->registration->properties.Value_unlocked(Constants::SERVICE_ID)
+               .first;
     assert(anyId1.Type() == typeid(long int));
   }
 
@@ -157,11 +160,14 @@ bool ServiceReferenceBase::operator<(
   {
     auto l = reference.d.load()->registration->properties.Lock();
     US_UNUSED(l);
-    anyR2 = reference.d.load()->registration->properties.Value_unlocked(
-      Constants::SERVICE_RANKING);
+    anyR2 =
+      reference.d.load()
+        ->registration->properties.Value_unlocked(Constants::SERVICE_RANKING)
+        .first;
     assert(anyR2.Empty() || anyR2.Type() == typeid(int));
-    anyId2 = reference.d.load()->registration->properties.Value_unlocked(
-      Constants::SERVICE_ID);
+    anyId2 = reference.d.load()
+               ->registration->properties.Value_unlocked(Constants::SERVICE_ID)
+               .first;
     assert(anyId2.Type() == typeid(long int));
   }
 

diff --git a/framework/src/service/ServiceReferenceBasePrivate.cpp b/framework/src/service/ServiceReferenceBasePrivate.cpp
@@ -87,7 +87,8 @@ InterfaceMapConstPtr ServiceReferenceBasePrivate::GetServiceFromFactory(
     std::vector<std::string> classes =
       (registration->properties.Lock(),
        any_cast<std::vector<std::string>>(
-         registration->properties.Value_unlocked(Constants::OBJECTCLASS)));
+         registration->properties.Value_unlocked(Constants::OBJECTCLASS)
+           .first));
     for (auto clazz : classes) {
       if (smap->find(clazz) == smap->end() &&
           clazz != "org.cppmicroservices.factory") {

diff --git a/framework/src/service/ServiceRegistrationBase.cpp b/framework/src/service/ServiceRegistrationBase.cpp
@@ -49,7 +49,8 @@ ServiceRegistrationBase::ServiceRegistrationBase(
     ++d->ref;
 }
 
-ServiceRegistrationBase::ServiceRegistrationBase(ServiceRegistrationBase&& reg) noexcept
+ServiceRegistrationBase::ServiceRegistrationBase(
+  ServiceRegistrationBase&& reg) noexcept
   : d(nullptr)
 {
   std::swap(d, reg.d);
@@ -148,13 +149,14 @@ void ServiceRegistrationBase::SetProperties(const ServiceProperties& props)
 
     auto l2 = d->properties.Lock();
     US_UNUSED(l2);
+
     auto propsCopy(props);
     propsCopy[Constants::SERVICE_ID] =
-      d->properties.Value_unlocked(Constants::SERVICE_ID);
-    objectClasses = d->properties.Value_unlocked(Constants::OBJECTCLASS);
+      d->properties.Value_unlocked(Constants::SERVICE_ID).first;
+    objectClasses = d->properties.Value_unlocked(Constants::OBJECTCLASS).first;
     propsCopy[Constants::OBJECTCLASS] = objectClasses;
     propsCopy[Constants::SERVICE_SCOPE] =
-      d->properties.Value_unlocked(Constants::SERVICE_SCOPE);
+      d->properties.Value_unlocked(Constants::SERVICE_SCOPE).first;
 
     auto itr = propsCopy.find(Constants::SERVICE_RANKING);
     if (itr != propsCopy.end()) {
@@ -168,13 +170,14 @@ void ServiceRegistrationBase::SetProperties(const ServiceProperties& props)
       }
     }
 
-    auto oldRankAny = d->properties.Value_unlocked(Constants::SERVICE_RANKING);
+    auto oldRankAny =
+      d->properties.Value_unlocked(Constants::SERVICE_RANKING).first;
     if (!oldRankAny.Empty()) {
       // since the old ranking is extracted from existing service properties
       // stored in the service registry, no need to type check before casting
       old_rank = any_cast<int>(oldRankAny);
     }
-    d->properties = Properties(std::move(propsCopy));
+    d->properties = Properties(AnyMap(std::move(propsCopy)));
   }
   if (old_rank != new_rank) {
     auto classes = any_cast<std::vector<std::string>>(objectClasses);

diff --git a/framework/src/service/ServiceRegistry.cpp b/framework/src/service/ServiceRegistry.cpp
@@ -72,7 +72,7 @@ Properties ServiceRegistry::CreateServiceProperties(
       std::make_pair(Constants::SERVICE_SCOPE, Constants::SCOPE_SINGLETON));
   }
 
-  return Properties(std::move(props));
+  return Properties(AnyMap(std::move(props)));
 }
 
 ServiceRegistry::ServiceRegistry(CoreBundleContext* coreCtx)
@@ -273,10 +273,11 @@ void ServiceRegistry::RemoveServiceRegistration_unlocked(
   {
     auto l2 = sr.d->properties.Lock();
     US_UNUSED(l2);
-    assert(sr.d->properties.Value_unlocked(Constants::OBJECTCLASS).Type() ==
-           typeid(std::vector<std::string>));
+    assert(
+      sr.d->properties.Value_unlocked(Constants::OBJECTCLASS).first.Type() ==
+      typeid(std::vector<std::string>));
     classes = ref_any_cast<std::vector<std::string>>(
-      sr.d->properties.Value_unlocked(Constants::OBJECTCLASS));
+      sr.d->properties.Value_unlocked(Constants::OBJECTCLASS).first);
   }
   services.erase(sr);
   serviceRegistrations.erase(

diff --git a/framework/src/util/AnyMap.cpp b/framework/src/util/AnyMap.cpp
@@ -22,6 +22,7 @@
 
 #include "cppmicroservices/AnyMap.h"
 
+#include <cassert>
 #include <stdexcept>
 
 namespace cppmicroservices {
@@ -575,18 +576,36 @@ any_map::any_map(const ordered_any_map& m)
   map.o = new ordered_any_map(m);
 }
 
+any_map::any_map(ordered_any_map&& m)
+  : type(map_type::ORDERED_MAP)
+{
+  map.o = new ordered_any_map(std::move(m));
+}
+
 any_map::any_map(const unordered_any_map& m)
   : type(map_type::UNORDERED_MAP)
 {
   map.uo = new unordered_any_map(m);
 }
 
+any_map::any_map(unordered_any_map&& m)
+  : type(map_type::UNORDERED_MAP)
+{
+  map.uo = new unordered_any_map(std::move(m));
+}
+
 any_map::any_map(const unordered_any_cimap& m)
   : type(map_type::UNORDERED_MAP_CASEINSENSITIVE_KEYS)
 {
   map.uoci = new unordered_any_cimap(m);
 }
 
+any_map::any_map(unordered_any_cimap&& m)
+  : type(map_type::UNORDERED_MAP_CASEINSENSITIVE_KEYS)
+{
+  map.uoci = new unordered_any_cimap(std::move(m));
+}
+
 any_map::any_map(const any_map& m)
   : type(m.type)
 {
@@ -835,6 +854,78 @@ any_map::const_iterator any_map::find(const key_type& key) const
   }
 }
 
+any_map::ordered_any_map::const_iterator any_map::beginOM_TypeChecked() const
+{
+  assert(type == ORDERED_MAP && "You are calling beginOM_TypeChecked() on map "
+                                "whose type is not ORDERED_MAP.");
+  return map.o->begin();
+}
+
+any_map::ordered_any_map::const_iterator any_map::endOM_TypeChecked() const
+{
+  assert(type == ORDERED_MAP && "You are calling endOM_TypeChecked() on map "
+                                "whose type is not ORDERED_MAP.");
+  return map.o->end();
+}
+
+any_map::ordered_any_map::const_iterator any_map::findOM_TypeChecked(
+  const key_type& key) const
+{
+  assert(type == ORDERED_MAP && "You are calling findOM_TypeChecked() on map "
+                                "whose type is not ORDERED_MAP.");
+  return map.o->find(key);
+}
+
+any_map::unordered_any_map::const_iterator any_map::beginUO_TypeChecked() const
+{
+  assert(type == UNORDERED_MAP &&
+         "You are calling beginUO_TypeChecked() on map "
+         "whose type is not UNORDERED_MAP.");
+  return map.uo->begin();
+}
+
+any_map::unordered_any_map::const_iterator any_map::endUO_TypeChecked() const
+{
+  assert(type == UNORDERED_MAP && "You are calling endUO_TypeChecked() on map "
+                                  "whose type is not UNORDERED_MAP.");
+  return map.uo->end();
+}
+
+any_map::unordered_any_map::const_iterator any_map::findUO_TypeChecked(
+  const key_type& key) const
+{
+  assert(type == UNORDERED_MAP && "You are calling findUO_TypeChecked() on map "
+                                  "whose type is not UNORDERED_MAP.");
+  return map.uo->find(key);
+}
+
+any_map::unordered_any_cimap::const_iterator any_map::beginUOCI_TypeChecked()
+  const
+{
+  assert(type == UNORDERED_MAP_CASEINSENSITIVE_KEYS &&
+         "You are calling beginUOCI_TypeChecked() on map "
+         "whose type is not UNORDERED_MAP_CASEINSENSITIVE_KEYS.");
+  return map.uoci->begin();
+}
+
+any_map::unordered_any_cimap::const_iterator any_map::endUOCI_TypeChecked()
+  const
+{
+  assert(type == UNORDERED_MAP_CASEINSENSITIVE_KEYS &&
+         "You are calling endUOCI_TypeChecked() on map "
+         "whose type is not UNORDERED_MAP_CASEINSENSITIVE_KEYS.");
+  return map.uoci->end();
+}
+
+any_map::unordered_any_cimap::const_iterator any_map::findUOCI_TypeChecked(
+  const key_type& key) const
+{
+  assert(type == UNORDERED_MAP_CASEINSENSITIVE_KEYS &&
+         "You are calling findUOCI_TypeChecked() on map "
+         "whose type is not UNORDERED_MAP_CASEINSENSITIVE_KEYS.");
+  return map.uoci->find(key);
+}
+
 any_map::size_type any_map::erase(const key_type& key)
 {
   switch (type) {