[jira] [Commented] (MESOS-5921) `validate` is a bit heavy to check negative scalar resource

["Guangya Liu (JIRA)" <ji...@apache.org>]

    [ 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)