[ https://issues.apache.org/jira/browse/MESOS-5921?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15397700#comment-15397700 ]
Guangya Liu commented on MESOS-5921: ------------------------------------ [~bmahler], did some checking for this and seems we can keep the current logic of of {{Resources::subtract}} using {{Resources::validate}} as this function can return very quickly when encounter negative scalar resources. What do you think? Thanks. {code} Option<Error> Resources::validate(const Resource& resource) { if (resource.name().empty()) { return Error("Empty resource name"); } if (!Value::Type_IsValid(resource.type())) { return Error("Invalid resource type"); } if (resource.type() == Value::SCALAR) { if (!resource.has_scalar() || resource.has_ranges() || resource.has_set()) { return Error("Invalid scalar resource"); } if (resource.scalar().value() < 0) { return Error("Invalid scalar resource: value < 0"); <<<<<< Return here if the scalar resource is negative and thus will not do other checking. } } else if (resource.type() == Value::RANGES) { ...... } else if (resource.type() == Value::SET) { ...... } else { // Resource doesn't support TEXT or other value types. return Error("Unsupported resource type"); } ...... } {code} I also did some test with following code diff and found that the performance was almost not changed for operating 1000 port resources. Code diff. {code} --- a/include/mesos/resources.hpp +++ b/include/mesos/resources.hpp @@ -396,6 +396,9 @@ private: // ensure this is warranted. bool _contains(const Resource& that) const; + // Check if the resource is a negative scalar resource. + bool isNegative(const Resource& r) const; + // Similar to the public 'find', but only for a single Resource // object. The target resource may span multiple roles, so this // returns Resources. diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 2878ace..b1259b9 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -1296,6 +1296,17 @@ bool Resources::_contains(const Resource& that) const } +bool Resources::isNegative(const Resource& r) const +{ + if (r.type() == Value::SCALAR && + r.scalar().value() < 0) { + return true; + } + + return false; +} + + Option<Resources> Resources::find(const Resource& target) const { Resources found; @@ -1442,10 +1453,8 @@ void Resources::subtract(const Resource& that) 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)) { + // Remove the resource if it becomes negative or empty. + if (isNegative(*resource) || 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 {code} Before fix: {code} [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 Took 2.730778secs to perform 1000 'total += r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 20.703045secs to perform 1000 'total.contains(r)' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 3.530712secs to perform 1000 'total -= r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 2.92716secs to perform 1000 'total = total + r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 3.489936secs to perform 1000 'total = total - r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 122368us to perform 1000 'r.nonRevocable()' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (33508 ms) [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (33508 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (33525 ms total) [ PASSED ] 1 test. {code} After fix: {code} [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 Took 2.657057secs to perform 1000 'total += r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 20.493614secs to perform 1000 'total.contains(r)' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 3.420194secs to perform 1000 'total -= r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 2.855921secs to perform 1000 'total = total + r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 3.463858secs to perform 1000 'total = total - r' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... Took 128458us to perform 1000 'r.nonRevocable()' operations on ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (33023 ms) [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (33023 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (33041 ms total) [ PASSED ] 1 test. {code} > `validate` is a bit heavy to check negative scalar resource > ----------------------------------------------------------- > > Key: MESOS-5921 > URL: https://issues.apache.org/jira/browse/MESOS-5921 > Project: Mesos > Issue Type: Bug > Reporter: Guangya Liu > Assignee: Guangya Liu > > When subtract resources finished, we need to call {{Resources::validate}} to > check if the scalar resource is negative so as to remove this resource if it > is negative. This is a bit heavy as the {{Resources::validate}} did many > validation stuffs, such as checking type, validating role, checking resource > name etc, all of them are not necessary. > We should introduce a new helper function {{isNegative}} to check if the > resource is a negative scalar resource. -- This message was sent by Atlassian JIRA (v6.3.4#6332)