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 <[email protected]> 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;
