[mesos] 01/02: Optimized Resources::shrink.

[bm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

bmahler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit de90b2b3078e06975ab2cccc061db821cfe7dda8
Author: Benjamin Mahler <bm...@apache.org>
AuthorDate: Thu Aug 22 17:41:28 2019 -0400

    Optimized Resources::shrink.
    
    Master:
    *HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/2
    Made 3500 allocations in 30.37 secs
    Made 0 allocation in 27.05 secs
    
    Master + this patch:
    *HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/2
    Made 3500 allocations in 24.15 secs
    Made 0 allocation in 20.48 secs
    
    Review: https://reviews.apache.org/r/71353
---
 include/mesos/resources.hpp    |  9 +++++
 include/mesos/v1/resources.hpp |  9 +++++
 src/common/resources.cpp       | 78 ++++++++++++++++++++++++------------------
 src/v1/resources.cpp           | 78 ++++++++++++++++++++++++------------------
 4 files changed, 106 insertions(+), 68 deletions(-)

diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp
index a6a052b..e5e87a0 100644
--- a/include/mesos/resources.hpp
+++ b/include/mesos/resources.hpp
@@ -821,6 +821,15 @@ hashmap<Key, Resources> operator+(
 }
 
 
+// Tests if `right` is contained in `left`, note that most
+// callers should just make use of `Resources::contains(...)`.
+// However, if dealing only with singular `Resource` objects,
+// this has lower overhead.
+//
+// NOTE: `left` and `right` must be valid resource objects.
+bool contains(const Resource& left, const Resource& right);
+
+
 /**
  * Represents a resource conversion, usually as a result of an offer
  * operation. See more details in `Resources::apply` method.
diff --git a/include/mesos/v1/resources.hpp b/include/mesos/v1/resources.hpp
index e43d1fb..6a9751a 100644
--- a/include/mesos/v1/resources.hpp
+++ b/include/mesos/v1/resources.hpp
@@ -816,6 +816,15 @@ hashmap<Key, Resources> operator+(
 }
 
 
+// Tests if `right` is contained in `left`, note that most
+// callers should just make use of `Resources::contains(...)`.
+// However, if dealing only with singular `Resource` objects,
+// this has lower overhead.
+//
+// NOTE: `left` and `right` must be valid resource objects.
+bool contains(const Resource& left, const Resource& right);
+
+
 /**
  * Represents a resource conversion, usually as a result of an offer
  * operation. See more details in `Resources::apply` method.
diff --git a/src/common/resources.cpp b/src/common/resources.cpp
index fc7e86b..bfa9f3e 100644
--- a/src/common/resources.cpp
+++ b/src/common/resources.cpp
@@ -562,29 +562,6 @@ static bool subtractable(const Resource& left, const Resource& right)
 }
 
 
-// Tests if "right" is contained in "left".
-static bool contains(const Resource& left, const Resource& right)
-{
-  // NOTE: This is a necessary condition for 'contains'.
-  // 'subtractable' will verify name, role, type, ReservationInfo,
-  // DiskInfo, SharedInfo, RevocableInfo, and ResourceProviderID
-  // compatibility.
-  if (!subtractable(left, right)) {
-    return false;
-  }
-
-  if (left.type() == Value::SCALAR) {
-    return right.scalar() <= left.scalar();
-  } else if (left.type() == Value::RANGES) {
-    return right.ranges() <= left.ranges();
-  } else if (left.type() == Value::SET) {
-    return right.set() <= left.set();
-  } else {
-    return false;
-  }
-}
-
-
 /**
  * Checks that a Resources object is valid for command line specification.
  *
@@ -1314,19 +1291,30 @@ bool Resources::shrink(Resource* resource, const Value::Scalar& target)
     return true; // Already within target.
   }
 
-  Resource copy = *resource;
-  copy.mutable_scalar()->CopyFrom(target);
+  // Some `disk` resources (e.g. MOUNT disk) are indivisible.
+  // We use a containement check to verify this. Specifically,
+  // if it contains a smaller version of itself, then it can
+  // safely be chopped into a smaller amount.
+  //
+  // NOTE: If additional types of resources become indivisible,
+  // this code needs updating!
+  if (resource->has_disk()) {
+    Resource original = *resource;
 
-  // Some resources (e.g. MOUNT disk) are indivisible. We use
-  // a containement check to verify this. Specifically, if a
-  // contains a smaller version of itself, then it can safely
-  // be chopped into a smaller amount.
-  if (Resources(*resource).contains(copy)) {
-    resource->CopyFrom(copy);
-    return true;
+    Value::Scalar oldScalar = resource->scalar();
+    *resource->mutable_scalar() = target;
+
+    if (mesos::contains(original, *resource)) {
+      return true;
+    }
+
+    // Restore the old value.
+    *resource->mutable_scalar() = std::move(oldScalar);
+    return false;
   }
 
-  return false;
+  *resource->mutable_scalar() = target;
+  return true;
 }
 
 
@@ -1370,7 +1358,7 @@ bool Resources::Resource_::contains(const Resource_& that) const
   }
 
   // For non-shared resources just compare the protobufs.
-  return internal::contains(resource, that.resource);
+  return mesos::contains(resource, that.resource);
 }
 
 
@@ -2658,6 +2646,28 @@ ostream& operator<<(
 }
 
 
+bool contains(const Resource& left, const Resource& right)
+{
+  // NOTE: This is a necessary condition for 'contains'.
+  // 'subtractable' will verify name, role, type, ReservationInfo,
+  // DiskInfo, SharedInfo, RevocableInfo, and ResourceProviderID
+  // compatibility.
+  if (!internal::subtractable(left, right)) {
+    return false;
+  }
+
+  if (left.type() == Value::SCALAR) {
+    return right.scalar() <= left.scalar();
+  } else if (left.type() == Value::RANGES) {
+    return right.ranges() <= left.ranges();
+  } else if (left.type() == Value::SET) {
+    return right.set() <= left.set();
+  } else {
+    return false;
+  }
+}
+
+
 Try<Resources> ResourceConversion::apply(const Resources& resources) const
 {
   Resources result = resources;
diff --git a/src/v1/resources.cpp b/src/v1/resources.cpp
index 88da0a1..28aa62b 100644
--- a/src/v1/resources.cpp
+++ b/src/v1/resources.cpp
@@ -555,29 +555,6 @@ static bool subtractable(const Resource& left, const Resource& right)
 }
 
 
-// Tests if "right" is contained in "left".
-static bool contains(const Resource& left, const Resource& right)
-{
-  // NOTE: This is a necessary condition for 'contains'.
-  // 'subtractable' will verify name, role, type, ReservationInfo,
-  // DiskInfo, SharedInfo, RevocableInfo, and ResourceProviderID
-  // compatibility.
-  if (!subtractable(left, right)) {
-    return false;
-  }
-
-  if (left.type() == Value::SCALAR) {
-    return right.scalar() <= left.scalar();
-  } else if (left.type() == Value::RANGES) {
-    return right.ranges() <= left.ranges();
-  } else if (left.type() == Value::SET) {
-    return right.set() <= left.set();
-  } else {
-    return false;
-  }
-}
-
-
 /**
  * Checks that a Resources object is valid for command line specification.
  *
@@ -1334,19 +1311,30 @@ bool Resources::shrink(Resource* resource, const Value::Scalar& target)
     return true; // Already within target.
   }
 
-  Resource copy = *resource;
-  copy.mutable_scalar()->CopyFrom(target);
+  // Some `disk` resources (e.g. MOUNT disk) are indivisible.
+  // We use a containement check to verify this. Specifically,
+  // if it contains a smaller version of itself, then it can
+  // safely be chopped into a smaller amount.
+  //
+  // NOTE: If additional types of resources become indivisible,
+  // this code needs updating!
+  if (resource->has_disk()) {
+    Resource original = *resource;
 
-  // Some resources (e.g. MOUNT disk) are indivisible. We use
-  // a containement check to verify this. Specifically, if a
-  // contains a smaller version of itself, then it can safely
-  // be chopped into a smaller amount.
-  if (Resources(*resource).contains(copy)) {
-    resource->CopyFrom(copy);
-    return true;
+    Value::Scalar oldScalar = resource->scalar();
+    *resource->mutable_scalar() = target;
+
+    if (mesos::v1::contains(original, *resource)) {
+      return true;
+    }
+
+    // Restore the old value.
+    *resource->mutable_scalar() = std::move(oldScalar);
+    return false;
   }
 
-  return false;
+  *resource->mutable_scalar() = target;
+  return true;
 }
 
 
@@ -1390,7 +1378,7 @@ bool Resources::Resource_::contains(const Resource_& that) const
   }
 
   // For non-shared resources just compare the protobufs.
-  return internal::contains(resource, that.resource);
+  return mesos::v1::contains(resource, that.resource);
 }
 
 
@@ -2662,6 +2650,28 @@ ostream& operator<<(
 }
 
 
+bool contains(const Resource& left, const Resource& right)
+{
+  // NOTE: This is a necessary condition for 'contains'.
+  // 'subtractable' will verify name, role, type, ReservationInfo,
+  // DiskInfo, SharedInfo, RevocableInfo, and ResourceProviderID
+  // compatibility.
+  if (!internal::subtractable(left, right)) {
+    return false;
+  }
+
+  if (left.type() == Value::SCALAR) {
+    return right.scalar() <= left.scalar();
+  } else if (left.type() == Value::RANGES) {
+    return right.ranges() <= left.ranges();
+  } else if (left.type() == Value::SET) {
+    return right.set() <= left.set();
+  } else {
+    return false;
+  }
+}
+
+
 Try<Resources> ResourceConversion::apply(const Resources& resources) const
 {
   Resources result = resources;