mesos git commit: Improved performance of filters in the allocator.

[bm...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master c004a6459 -> fdc4536c2


Improved performance of filters in the allocator.

When frameworks refuse a lot of resources, the list of filters gets
long. Since the filters are per-slave,
HierarchicalAllocatorProcess::isFiltered spends a lot of time just
comparing SlaveID (which tend to be long strings). Eliminate this
whole problem by organizing the filters by SlaveID in the first
place.

Review: https://reviews.apache.org/r/37913


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/fdc4536c
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/fdc4536c
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/fdc4536c

Branch: refs/heads/master
Commit: fdc4536c2125e7502ca6b37c14afccbd7917d974
Parents: c004a64
Author: James Peach <jp...@apache.org>
Authored: Tue Sep 1 18:29:29 2015 -0700
Committer: Benjamin Mahler <be...@gmail.com>
Committed: Tue Sep 1 18:29:29 2015 -0700

----------------------------------------------------------------------
 src/master/allocator/mesos/hierarchical.hpp | 48 ++++++++++++++----------
 1 file changed, 28 insertions(+), 20 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/fdc4536c/src/master/allocator/mesos/hierarchical.hpp
----------------------------------------------------------------------
diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp
index 38f8fd2..cb4020d 100644
--- a/src/master/allocator/mesos/hierarchical.hpp
+++ b/src/master/allocator/mesos/hierarchical.hpp
@@ -171,7 +171,10 @@ protected:
   void allocate(const hashset<SlaveID>& slaveIds);
 
   // Remove a filter for the specified framework.
-  void expire(const FrameworkID& frameworkId, Filter* filter);
+  void expire(
+      const FrameworkID& frameworkId,
+      const SlaveID& slaveId,
+      Filter* filter);
 
   // Checks whether the slave is whitelisted.
   bool isWhitelisted(const SlaveID& slaveId);
@@ -219,7 +222,8 @@ protected:
     // Whether the framework desires revocable resources.
     bool revocable;
 
-    hashset<Filter*> filters; // Active filters for the framework.
+    // Active filters for the framework.
+    hashmap<SlaveID, hashset<Filter*>> filters;
   };
 
   double _event_queue_dispatches()
@@ -287,7 +291,7 @@ class Filter
 public:
   virtual ~Filter() {}
 
-  virtual bool filter(const SlaveID& slaveId, const Resources& resources) = 0;
+  virtual bool filter(const Resources& resources) = 0;
 };
 
 
@@ -295,24 +299,21 @@ class RefusedFilter: public Filter
 {
 public:
   RefusedFilter(
-      const SlaveID& _slaveId,
       const Resources& _resources,
       const process::Timeout& _timeout)
-    : slaveId(_slaveId), resources(_resources), timeout(_timeout) {}
+    : resources(_resources), timeout(_timeout) {}
 
-  virtual bool filter(const SlaveID& _slaveId, const Resources& _resources)
+  virtual bool filter(const Resources& _resources)
   {
     // TODO(jieyu): Consider separating the superset check for regular
     // and revocable resources. For example, frameworks might want
     // more revocable resources only or non-revocable resources only,
     // but currently the filter only expires if there is more of both
     // revocable and non-revocable resources.
-    return slaveId == _slaveId &&
-           resources.contains(_resources) && // Refused resources are superset.
+    return resources.contains(_resources) && // Refused resources are superset.
            timeout.remaining() > Seconds(0);
   }
 
-  const SlaveID slaveId;
   const Resources resources;
   const process::Timeout timeout;
 };
@@ -856,13 +857,12 @@ HierarchicalAllocatorProcess<RoleSorter, FrameworkSorter>::recoverResources(
 
     // Create a new filter and delay its expiration.
     Filter* filter = new RefusedFilter(
-        slaveId,
         resources,
         process::Timeout::in(seconds.get()));
 
-    frameworks[frameworkId].filters.insert(filter);
+    frameworks[frameworkId].filters[slaveId].insert(filter);
 
-    delay(seconds.get(), self(), &Self::expire, frameworkId, filter);
+    delay(seconds.get(), self(), &Self::expire, frameworkId, slaveId, filter);
   }
 }
 
@@ -1021,6 +1021,7 @@ template <class RoleSorter, class FrameworkSorter>
 void
 HierarchicalAllocatorProcess<RoleSorter, FrameworkSorter>::expire(
     const FrameworkID& frameworkId,
+    const SlaveID& slaveId,
     Filter* filter)
 {
   // The filter might have already been removed (e.g., if the
@@ -1029,8 +1030,12 @@ HierarchicalAllocatorProcess<RoleSorter, FrameworkSorter>::expire(
   // keep the address from getting reused possibly causing premature
   // expiration).
   if (frameworks.contains(frameworkId) &&
-      frameworks[frameworkId].filters.contains(filter)) {
-    frameworks[frameworkId].filters.erase(filter);
+      frameworks[frameworkId].filters.contains(slaveId) &&
+      frameworks[frameworkId].filters[slaveId].contains(filter)) {
+    frameworks[frameworkId].filters[slaveId].erase(filter);
+    if (frameworks[frameworkId].filters[slaveId].empty()) {
+      frameworks[frameworkId].filters.erase(slaveId);
+    }
   }
 
   delete filter;
@@ -1069,14 +1074,17 @@ HierarchicalAllocatorProcess<RoleSorter, FrameworkSorter>::isFiltered(
     return true;
   }
 
-  foreach (Filter* filter, frameworks[frameworkId].filters) {
-    if (filter->filter(slaveId, resources)) {
-      VLOG(1) << "Filtered " << resources
-              << " on slave " << slaveId
-              << " for framework " << frameworkId;
-      return true;
+  if (frameworks[frameworkId].filters.contains(slaveId)) {
+    foreach (Filter* filter, frameworks[frameworkId].filters[slaveId]) {
+      if (filter->filter(resources)) {
+        VLOG(1) << "Filtered " << resources
+                << " on slave " << slaveId
+                << " for framework " << frameworkId;
+        return true;
+      }
     }
   }
+
   return false;
 }