[Refactor] Replace FractionalResourceQuantity with FixedPoint (ray-pr…

…oject#16052)

* refactor

* fix

* fix compilation

* fix

* fix cross-platform compilation

* lint

* fix test

* Revert "fix test"

This reverts commit 0ff23b1.

* change rounding to truncating

* Update BUILD.bazel

Co-authored-by: Eric Liang <[email protected]>

BUILD.bazel

@@ -330,12 +330,20 @@ cc_library(
         exclude = [
             "src/ray/common/**/*_test.cc",
         ],
-    ),
+    ) + [
+        "src/ray/raylet/scheduling/cluster_resource_data.cc",
+        "src/ray/raylet/scheduling/fixed_point.cc",
+        "src/ray/raylet/scheduling/scheduling_ids.cc",
+    ],
     hdrs = glob(
         [
             "src/ray/common/**/*.h",
         ],
-    ),
+    ) + [
+        "src/ray/raylet/scheduling/cluster_resource_data.h",
+        "src/ray/raylet/scheduling/fixed_point.h",
+        "src/ray/raylet/scheduling/scheduling_ids.h",
+    ],
     copts = COPTS,
     strip_include_prefix = "src",
     visibility = ["//visibility:public"],

src/ray/common/task/scheduling_resources.h

@@ -7,11 +7,12 @@
 
 #include "ray/common/id.h"
 #include "ray/raylet/format/node_manager_generated.h"
+#include "ray/raylet/scheduling/cluster_resource_data.h"
 
 namespace ray {
 
 /// Conversion factor that is the amount in internal units is equivalent to
-/// one actual resource. Multiply to convert from actual to interal and
+/// one actual resource. Multiply to convert from actual to internal and
 /// divide to convert from internal to actual.
 constexpr double kResourceConversionFactor = 10000;
 
@@ -20,48 +21,6 @@ const std::string kGPU_ResourceLabel = "GPU";
 const std::string kObjectStoreMemory_ResourceLabel = "object_store_memory";
 const std::string kMemory_ResourceLabel = "memory";
 
-/// \class FractionalResourceQuantity
-/// \brief Converts the resource quantities to an internal representation to
-/// avoid machine precision errors.
-class FractionalResourceQuantity {
- public:
-  /// \brief Construct a FractionalResourceQuantity representing zero
-  /// resources. This constructor is used by std::unordered_map when we try
-  /// to add a new FractionalResourceQuantity in ResourceSets.
-  FractionalResourceQuantity();
-
-  /// \brief Construct a FractionalResourceQuantity representing
-  /// resource_quantity.
-  FractionalResourceQuantity(double resource_quantity);
-
-  /// \brief Addition of FractionalResourceQuantity.
-  const FractionalResourceQuantity operator+(const FractionalResourceQuantity &rhs) const;
-
-  /// \brief Subtraction of FractionalResourceQuantity.
-  const FractionalResourceQuantity operator-(const FractionalResourceQuantity &rhs) const;
-
-  /// \brief Addition and assignment of FractionalResourceQuantity.
-  void operator+=(const FractionalResourceQuantity &rhs);
-
-  /// \brief Subtraction and assignment of FractionalResourceQuantity.
-  void operator-=(const FractionalResourceQuantity &rhs);
-
-  bool operator==(const FractionalResourceQuantity &rhs) const;
-  bool operator!=(const FractionalResourceQuantity &rhs) const;
-  bool operator<(const FractionalResourceQuantity &rhs) const;
-  bool operator>(const FractionalResourceQuantity &rhs) const;
-  bool operator<=(const FractionalResourceQuantity &rhs) const;
-  bool operator>=(const FractionalResourceQuantity &rhs) const;
-
-  /// \brief Return actual resource amount as a double.
-  double ToDouble() const;
-
- private:
-  /// The resource quantity represented as 1/kResourceConversionFactor-th of a
-  /// unit.
-  int64_t resource_quantity_;
-};
-
 /// \class ResourceSet
 /// \brief Encapsulates and operates on a set of resources, including CPUs,
 /// GPUs, and custom labels.
@@ -76,8 +35,7 @@ class ResourceSet {
   ResourceSet();
 
   /// \brief Constructs ResourceSet from the specified resource map.
-  ResourceSet(
-      const std::unordered_map<std::string, FractionalResourceQuantity> &resource_map);
+  ResourceSet(const std::unordered_map<std::string, FixedPoint> &resource_map);
 
   /// \brief Constructs ResourceSet from the specified resource map.
   ResourceSet(const std::unordered_map<std::string, double> &resource_map);
@@ -121,8 +79,7 @@ class ResourceSet {
   /// \param resource_name: name/label of the resource to add.
   /// \param capacity: numeric capacity value for the resource to add.
   /// \return True, if the resource was successfully added. False otherwise.
-  void AddOrUpdateResource(const std::string &resource_name,
-                           const FractionalResourceQuantity &capacity);
+  void AddOrUpdateResource(const std::string &resource_name, const FixedPoint &capacity);
 
   /// \brief Delete a resource from the resource set.
   ///
@@ -168,7 +125,7 @@ class ResourceSet {
   /// \param resource_name: Resource name for which capacity is requested.
   /// \return The capacity value associated with the specified resource, zero if resource
   /// does not exist.
-  FractionalResourceQuantity GetResource(const std::string &resource_name) const;
+  FixedPoint GetResource(const std::string &resource_name) const;
 
   /// Return the number of CPUs.
   ///
@@ -182,25 +139,24 @@ class ResourceSet {
 
   // TODO(atumanov): implement const_iterator class for the ResourceSet container.
   // TODO(williamma12): Make sure that everywhere we use doubles we don't
-  // convert it back to FractionalResourceQuantity.
+  // convert it back to FixedPoint.
   /// \brief Return a map of the resource and size in doubles. Note, size is in
   /// regular units and does not need to be multiplied by kResourceConversionFactor.
   ///
   /// \return map of resource in string to size in double.
   const std::unordered_map<std::string, double> GetResourceMap() const;
 
-  /// \brief Return a map of the resource and size in FractionalResourceQuantity. Note,
+  /// \brief Return a map of the resource and size in FixedPoint. Note,
   /// size is in kResourceConversionFactor of a unit.
   ///
-  /// \return map of resource in string to size in FractionalResourceQuantity.
-  const std::unordered_map<std::string, FractionalResourceQuantity>
-      &GetResourceAmountMap() const;
+  /// \return map of resource in string to size in FixedPoint.
+  const std::unordered_map<std::string, FixedPoint> &GetResourceAmountMap() const;
 
   const std::string ToString() const;
 
  private:
   /// Resource capacity map.
-  std::unordered_map<std::string, FractionalResourceQuantity> resource_capacity_;
+  std::unordered_map<std::string, FixedPoint> resource_capacity_;
 };
 
 /// \class ResourceIds
@@ -228,16 +184,14 @@ class ResourceIds {
   /// \brief Constructs ResourceIds with a given set of fractional IDs.
   ///
   /// \param fractional_ids: A vector of the resource IDs that are partially available.
-  explicit ResourceIds(
-      const std::vector<std::pair<int64_t, FractionalResourceQuantity>> &fractional_ids);
+  explicit ResourceIds(const std::vector<std::pair<int64_t, FixedPoint>> &fractional_ids);
 
   /// \brief Constructs ResourceIds with a given set of whole IDs and fractional IDs.
   ///
   /// \param whole_ids: A vector of the resource IDs that are completely available.
   /// \param fractional_ids: A vector of the resource IDs that are partially available.
-  ResourceIds(
-      const std::vector<int64_t> &whole_ids,
-      const std::vector<std::pair<int64_t, FractionalResourceQuantity>> &fractional_ids);
+  ResourceIds(const std::vector<int64_t> &whole_ids,
+              const std::vector<std::pair<int64_t, FixedPoint>> &fractional_ids);
 
   /// \brief Check if we have at least the requested amount.
   ///
@@ -249,14 +203,14 @@ class ResourceIds {
   ///
   /// \param resource_quantity Either a whole number or a fraction less than 1.
   /// \return True if there we have enough of the resource.
-  bool Contains(const FractionalResourceQuantity &resource_quantity) const;
+  bool Contains(const FixedPoint &resource_quantity) const;
 
   /// \brief Acquire the requested amount of the resource.
   ///
   /// \param resource_quantity The amount to acquire. Either a whole number or a
   /// fraction less than 1.
   /// \return A ResourceIds representing the specific acquired IDs.
-  ResourceIds Acquire(const FractionalResourceQuantity &resource_quantity);
+  ResourceIds Acquire(const FixedPoint &resource_quantity);
 
   /// \brief Return some resource IDs.
   ///
@@ -278,8 +232,7 @@ class ResourceIds {
   /// \brief Return just the fractional IDs.
   ///
   /// \return The fractional IDs.
-  const std::vector<std::pair<int64_t, FractionalResourceQuantity>> &FractionalIds()
-      const;
+  const std::vector<std::pair<int64_t, FixedPoint>> &FractionalIds() const;
 
   /// \brief Check if ResourceIds has any resources.
   ///
@@ -289,7 +242,7 @@ class ResourceIds {
   /// \brief Return the total quantity of resources, ignoring the specific IDs.
   ///
   /// \return The total quantity of the resource.
-  FractionalResourceQuantity TotalQuantity() const;
+  FixedPoint TotalQuantity() const;
 
   /// \brief Return a string representation of the object.
   ///
@@ -327,10 +280,10 @@ class ResourceIds {
   std::vector<int64_t> whole_ids_;
   /// A vector of pairs of resource ID and a fraction of that ID (the fraction
   /// is at least zero and strictly less than 1).
-  std::vector<std::pair<int64_t, FractionalResourceQuantity>> fractional_ids_;
+  std::vector<std::pair<int64_t, FixedPoint>> fractional_ids_;
   /// Quantity to track the total capacity of the resource, since the whole_ids_ vector
   /// keeps changing
-  FractionalResourceQuantity total_capacity_;
+  FixedPoint total_capacity_;
   /// Quantity to track any pending decrements in capacity that weren't executed because
   /// of insufficient available resources. This backlog in cleared in the release method.
   int64_t decrement_backlog_;

src/ray/gcs/gcs_server/gcs_resource_manager.cc

@@ -159,7 +159,7 @@ void GcsResourceManager::HandleGetAllAvailableResources(
     rpc::AvailableResources resource;
     resource.set_node_id(iter.first.Binary());
     for (const auto &res : iter.second.GetAvailableResources().GetResourceAmountMap()) {
-      (*resource.mutable_resources_available())[res.first] = res.second.ToDouble();
+      (*resource.mutable_resources_available())[res.first] = res.second.Double();
     }
     reply->add_resources_list()->CopyFrom(resource);
   }

src/ray/gcs/gcs_server/gcs_resource_scheduler.cc

@@ -42,14 +42,14 @@ double LeastResourceScorer::Score(const ResourceSet &required_resources,
   return node_score;
 }
 
-double LeastResourceScorer::Calculate(const FractionalResourceQuantity &requested,
-                                      const FractionalResourceQuantity &available) {
-  RAY_CHECK(available >= 0) << "Available resource " << available.ToDouble()
+double LeastResourceScorer::Calculate(const FixedPoint &requested,
+                                      const FixedPoint &available) {
+  RAY_CHECK(available >= 0) << "Available resource " << available.Double()
                             << " should be nonnegative.";
   if (requested > available) {
     return -1;
   }
-  return (available - requested).ToDouble() / available.ToDouble();
+  return (available - requested).Double() / available.Double();
 }
 
 /////////////////////////////////////////////////////////////////////////////////////////

src/ray/gcs/gcs_server/gcs_resource_scheduler.h

@@ -66,8 +66,7 @@ class LeastResourceScorer : public NodeScorer {
   /// \param requested Quantity of one of the required resources.
   /// \param available Quantity of one of the available resources.
   /// \return Score of the node.
-  double Calculate(const FractionalResourceQuantity &requested,
-                   const FractionalResourceQuantity &available);
+  double Calculate(const FixedPoint &requested, const FixedPoint &available);
 };
 
 /// Gcs resource scheduler implementation.

src/ray/gcs/gcs_server/test/gcs_resource_scheduler_test.cc

@@ -50,7 +50,7 @@ class GcsResourceSchedulerTest : public ::testing::Test {
     const auto &cluster_resource = gcs_resource_manager_->GetClusterResources();
     auto iter = cluster_resource.find(node_id);
     ASSERT_TRUE(iter != cluster_resource.end());
-    ASSERT_EQ(iter->second.GetAvailableResources().GetResource(resource_name).ToDouble(),
+    ASSERT_EQ(iter->second.GetAvailableResources().GetResource(resource_name).Double(),
               resource_value);
   }
 

src/ray/raylet/scheduling/cluster_resource_data.cc

@@ -1,4 +1,5 @@
 #include "ray/raylet/scheduling/cluster_resource_data.h"
+#include "ray/common/task/scheduling_resources.h"
 
 const std::string resource_labels[] = {
     ray::kCPU_ResourceLabel, ray::kMemory_ResourceLabel, ray::kGPU_ResourceLabel,

src/ray/raylet/scheduling/cluster_resource_data.h

@@ -20,7 +20,6 @@
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
-#include "ray/common/task/scheduling_resources.h"
 #include "ray/raylet/scheduling/fixed_point.h"
 #include "ray/raylet/scheduling/scheduling_ids.h"
 #include "ray/util/logging.h"

src/ray/raylet/scheduling/fixed_point.cc

@@ -2,15 +2,17 @@
 
 #include <cmath>
 
-FixedPoint::FixedPoint(double d) {
-  // We need to round, not truncate because floating point multiplication can
-  // leave a number slightly smaller than the intended whole number.
-  i_ = (uint64_t)((d * RESOURCE_UNIT_SCALING) + 0.5);
-}
+FixedPoint::FixedPoint(double d) { i_ = (uint64_t)(d * RESOURCE_UNIT_SCALING); }
 
 FixedPoint::FixedPoint(int i) { i_ = (i * RESOURCE_UNIT_SCALING); }
 
-FixedPoint FixedPoint::operator+(FixedPoint const &ru) {
+FixedPoint::FixedPoint(uint32_t i) { i_ = (i * RESOURCE_UNIT_SCALING); }
+
+FixedPoint::FixedPoint(int64_t i) : FixedPoint((double)i) {}
+
+FixedPoint::FixedPoint(uint64_t i) : FixedPoint((double)i) {}
+
+FixedPoint FixedPoint::operator+(FixedPoint const &ru) const {
   FixedPoint res;
   res.i_ = i_ + ru.i_;
   return res;
@@ -21,7 +23,7 @@ FixedPoint FixedPoint::operator+=(FixedPoint const &ru) {
   return *this;
 }
 
-FixedPoint FixedPoint::operator-(FixedPoint const &ru) {
+FixedPoint FixedPoint::operator-(FixedPoint const &ru) const {
   FixedPoint res;
   res.i_ = i_ - ru.i_;
   return res;
@@ -38,13 +40,13 @@ FixedPoint FixedPoint::operator-() const {
   return res;
 }
 
-FixedPoint FixedPoint::operator+(double const d) {
+FixedPoint FixedPoint::operator+(double const d) const {
   FixedPoint res;
   res.i_ = i_ + (int64_t)(d * RESOURCE_UNIT_SCALING);
   return res;
 }
 
-FixedPoint FixedPoint::operator-(double const d) {
+FixedPoint FixedPoint::operator-(double const d) const {
   FixedPoint res;
   res.i_ = i_ - (int64_t)(d * RESOURCE_UNIT_SCALING);
   return res;
@@ -55,6 +57,16 @@ FixedPoint FixedPoint::operator=(double const d) {
   return *this;
 }
 
+FixedPoint FixedPoint::operator+=(double const d) {
+  i_ += (int64_t)(d * RESOURCE_UNIT_SCALING);
+  return *this;
+}
+
+FixedPoint FixedPoint::operator+=(int64_t const ru) {
+  *this += (double)ru;
+  return *this;
+}
+
 bool FixedPoint::operator<(FixedPoint const &ru1) const { return (i_ < ru1.i_); };
 bool FixedPoint::operator>(FixedPoint const &ru1) const { return (i_ > ru1.i_); };
 bool FixedPoint::operator<=(FixedPoint const &ru1) const { return (i_ <= ru1.i_); };

src/ray/raylet/scheduling/fixed_point.h

@@ -13,23 +13,30 @@ class FixedPoint {
  public:
   FixedPoint(double d = 0);
   FixedPoint(int i);
+  FixedPoint(uint32_t i);
+  FixedPoint(int64_t i);
+  FixedPoint(uint64_t i);
 
-  FixedPoint operator+(FixedPoint const &ru);
+  FixedPoint operator+(FixedPoint const &ru) const;
 
   FixedPoint operator+=(FixedPoint const &ru);
 
-  FixedPoint operator-(FixedPoint const &ru);
+  FixedPoint operator-(FixedPoint const &ru) const;
 
   FixedPoint operator-=(FixedPoint const &ru);
 
   FixedPoint operator-() const;
 
-  FixedPoint operator+(double const d);
+  FixedPoint operator+(double const d) const;
 
-  FixedPoint operator-(double const d);
+  FixedPoint operator-(double const d) const;
 
   FixedPoint operator=(double const d);
 
+  FixedPoint operator+=(double const d);
+
+  FixedPoint operator+=(int64_t const ru);
+
   bool operator<(FixedPoint const &ru1) const;
   bool operator>(FixedPoint const &ru1) const;
   bool operator<=(FixedPoint const &ru1) const;