mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bmah...@apache.org
Subject mesos git commit: Avoid unnecessary validation in Resources arithmetic.
Date Sat, 23 Jul 2016 02:29:54 GMT
Repository: mesos
Updated Branches:
  refs/heads/master 52c51ffb3 -> 40e87290e


Avoid unnecessary validation in Resources arithmetic.

The 'Resource' objects within the 'Resources' class are always
valid. However, we currently unnecessarily re-validate during
the += and -= operators with a 'Resources' right hand side. This
was done incidentally because these operators happened to just
call into the += and -= operators to add each 'Resource'. These
operators must validate, since they take the 'Resource' protobuf,
which could be invalid!

This updates the += and -= operators to both call into common
'add' and 'subtract' methods that do not perform validation. In
the case of performing a += or -= with a 'Resource', we perform
validation before calling into 'add' or 'subtract'.

This provides a significant performance improvement, as the
validation consumed a lot of time during 'Resources' arithmetic.

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


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

Branch: refs/heads/master
Commit: 40e87290e1d0154d41870d730604d814ba187fce
Parents: 52c51ff
Author: Guangya Liu <gyliu513@gmail.com>
Authored: Fri Jul 22 17:03:34 2016 -0700
Committer: Benjamin Mahler <bmahler@apache.org>
Committed: Fri Jul 22 19:29:37 2016 -0700

----------------------------------------------------------------------
 include/mesos/resources.hpp    |  5 +++
 include/mesos/v1/resources.hpp |  5 +++
 src/common/resources.cpp       | 89 +++++++++++++++++++++---------------
 src/v1/resources.cpp           | 90 ++++++++++++++++++++++---------------
 4 files changed, 119 insertions(+), 70 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/40e87290/include/mesos/resources.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp
index a557e97..88a9fea 100644
--- a/include/mesos/resources.hpp
+++ b/include/mesos/resources.hpp
@@ -396,6 +396,11 @@ private:
   // returns Resources.
   Option<Resources> find(const Resource& target) const;
 
+  // Validation-free versions of += and -= `Resource` operators.
+  // These can be used when `r` is already validated.
+  void add(const Resource& r);
+  void subtract(const Resource& r);
+
   google::protobuf::RepeatedPtrField<Resource> resources;
 };
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/40e87290/include/mesos/v1/resources.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/v1/resources.hpp b/include/mesos/v1/resources.hpp
index a5ba8fe..054ed00 100644
--- a/include/mesos/v1/resources.hpp
+++ b/include/mesos/v1/resources.hpp
@@ -396,6 +396,11 @@ private:
   // returns Resources.
   Option<Resources> find(const Resource& target) const;
 
+  // Validation-free versions of += and -= `Resource` operators.
+  // These can be used when `r` is already validated.
+  void add(const Resource& r);
+  void subtract(const Resource& r);
+
   google::protobuf::RepeatedPtrField<Resource> resources;
 };
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/40e87290/src/common/resources.cpp
----------------------------------------------------------------------
diff --git a/src/common/resources.cpp b/src/common/resources.cpp
index b1bd278..3dbff24 100644
--- a/src/common/resources.cpp
+++ b/src/common/resources.cpp
@@ -1373,24 +1373,33 @@ Resources Resources::operator+(const Resources& that) const
 }
 
 
-Resources& Resources::operator+=(const Resource& that)
+void Resources::add(const Resource& that)
 {
-  if (validate(that).isNone() && !isEmpty(that)) {
-    bool found = false;
-    foreach (Resource& resource, resources) {
-      if (internal::addable(resource, that)) {
-        resource += that;
-        found = true;
-        break;
-      }
-    }
+  if (isEmpty(that)) {
+    return;
+  }
 
-    // Cannot be combined with any existing Resource object.
-    if (!found) {
-      resources.Add()->CopyFrom(that);
+  bool found = false;
+  foreach (Resource& resource, resources) {
+    if (internal::addable(resource, that)) {
+      resource += that;
+      found = true;
+      break;
     }
   }
 
+  // Cannot be combined with any existing Resource object.
+  if (!found) {
+    resources.Add()->CopyFrom(that);
+  }
+}
+
+Resources& Resources::operator+=(const Resource& that)
+{
+  if (validate(that).isNone()) {
+    add(that);
+  }
+
   return *this;
 }
 
@@ -1398,7 +1407,7 @@ Resources& Resources::operator+=(const Resource& that)
 Resources& Resources::operator+=(const Resources& that)
 {
   foreach (const Resource& resource, that.resources) {
-    *this += resource;
+    add(resource);
   }
 
   return *this;
@@ -1421,31 +1430,41 @@ Resources Resources::operator-(const Resources& that) const
 }
 
 
-Resources& Resources::operator-=(const Resource& that)
+void Resources::subtract(const Resource& that)
 {
-  if (validate(that).isNone() && !isEmpty(that)) {
-    for (int i = 0; i < resources.size(); i++) {
-      Resource* resource = resources.Mutable(i);
-
-      if (internal::subtractable(*resource, that)) {
-        *resource -= that;
-
-        // Remove the resource if it becomes invalid or zero. We need
-        // to do the validation because we want to strip negative
-        // scalar Resource object.
-        if (validate(*resource).isSome() || isEmpty(*resource)) {
-          // As `resources` is not ordered, and erasing an element
-          // from the middle using `DeleteSubrange` is expensive, we
-          // swap with the last element and then shrink the
-          // 'RepeatedPtrField' by one.
-          resources.Mutable(i)->Swap(resources.Mutable(resources.size() - 1));
-          resources.RemoveLast();
-        }
+  if (isEmpty(that)) {
+    return;
+  }
 
-        break;
+  for (int i = 0; i < resources.size(); i++) {
+    Resource* resource = resources.Mutable(i);
+
+    if (internal::subtractable(*resource, that)) {
+      *resource -= that;
+
+      // Remove the resource if it becomes invalid or zero. We need
+      // to do the validation because we want to strip negative
+      // scalar Resource object.
+      if (validate(*resource).isSome() || isEmpty(*resource)) {
+        // As `resources` is not ordered, and erasing an element
+        // from the middle using `DeleteSubrange` is expensive, we
+        // swap with the last element and then shrink the
+        // 'RepeatedPtrField' by one.
+        resources.Mutable(i)->Swap(resources.Mutable(resources.size() - 1));
+        resources.RemoveLast();
       }
+
+      break;
     }
   }
+}
+
+
+Resources& Resources::operator-=(const Resource& that)
+{
+  if (validate(that).isNone()) {
+    subtract(that);
+  }
 
   return *this;
 }
@@ -1454,7 +1473,7 @@ Resources& Resources::operator-=(const Resource& that)
 Resources& Resources::operator-=(const Resources& that)
 {
   foreach (const Resource& resource, that.resources) {
-    *this -= resource;
+    subtract(resource);
   }
 
   return *this;

http://git-wip-us.apache.org/repos/asf/mesos/blob/40e87290/src/v1/resources.cpp
----------------------------------------------------------------------
diff --git a/src/v1/resources.cpp b/src/v1/resources.cpp
index 6d4ec75..3c85dc8 100644
--- a/src/v1/resources.cpp
+++ b/src/v1/resources.cpp
@@ -1376,24 +1376,34 @@ Resources Resources::operator+(const Resources& that) const
 }
 
 
-Resources& Resources::operator+=(const Resource& that)
+void Resources::add(const Resource& that)
 {
-  if (validate(that).isNone() && !isEmpty(that)) {
-    bool found = false;
-    foreach (Resource& resource, resources) {
-      if (internal::addable(resource, that)) {
-        resource += that;
-        found = true;
-        break;
-      }
-    }
+  if (isEmpty(that)) {
+    return;
+  }
 
-    // Cannot be combined with any existing Resource object.
-    if (!found) {
-      resources.Add()->CopyFrom(that);
+  bool found = false;
+  foreach (Resource& resource, resources) {
+    if (internal::addable(resource, that)) {
+      resource += that;
+      found = true;
+      break;
     }
   }
 
+  // Cannot be combined with any existing Resource object.
+  if (!found) {
+    resources.Add()->CopyFrom(that);
+  }
+}
+
+
+Resources& Resources::operator+=(const Resource& that)
+{
+  if (validate(that).isNone()) {
+    add(that);
+  }
+
   return *this;
 }
 
@@ -1401,7 +1411,7 @@ Resources& Resources::operator+=(const Resource& that)
 Resources& Resources::operator+=(const Resources& that)
 {
   foreach (const Resource& resource, that.resources) {
-    *this += resource;
+    add(resource);
   }
 
   return *this;
@@ -1424,31 +1434,41 @@ Resources Resources::operator-(const Resources& that) const
 }
 
 
-Resources& Resources::operator-=(const Resource& that)
+void Resources::subtract(const Resource& that)
 {
-  if (validate(that).isNone() && !isEmpty(that)) {
-    for (int i = 0; i < resources.size(); i++) {
-      Resource* resource = resources.Mutable(i);
-
-      if (internal::subtractable(*resource, that)) {
-        *resource -= that;
-
-        // Remove the resource if it becomes invalid or zero. We need
-        // to do the validation because we want to strip negative
-        // scalar Resource object.
-        if (validate(*resource).isSome() || isEmpty(*resource)) {
-          // As `resources` is not ordered, and erasing an element
-          // from the middle using `DeleteSubrange` is expensive, we
-          // swap with the last element and then shrink the
-          // 'RepeatedPtrField' by one.
-          resources.Mutable(i)->Swap(resources.Mutable(resources.size() - 1));
-          resources.RemoveLast();
-        }
+  if (isEmpty(that)) {
+    return;
+  }
 
-        break;
+  for (int i = 0; i < resources.size(); i++) {
+    Resource* resource = resources.Mutable(i);
+
+    if (internal::subtractable(*resource, that)) {
+      *resource -= that;
+
+      // Remove the resource if it becomes invalid or zero. We need
+      // to do the validation because we want to strip negative
+      // scalar Resource object.
+      if (validate(*resource).isSome() || isEmpty(*resource)) {
+        // As `resources` is not ordered, and erasing an element
+        // from the middle using `DeleteSubrange` is expensive, we
+        // swap with the last element and then shrink the
+        // 'RepeatedPtrField' by one.
+        resources.Mutable(i)->Swap(resources.Mutable(resources.size() - 1));
+        resources.RemoveLast();
       }
+
+      break;
     }
   }
+}
+
+
+Resources& Resources::operator-=(const Resource& that)
+{
+  if (validate(that).isNone()) {
+    subtract(that);
+  }
 
   return *this;
 }
@@ -1457,7 +1477,7 @@ Resources& Resources::operator-=(const Resource& that)
 Resources& Resources::operator-=(const Resources& that)
 {
   foreach (const Resource& resource, that.resources) {
-    *this -= resource;
+    subtract(resource);
   }
 
   return *this;


Mime
View raw message