Merge pull request #1101 from DARMA-tasking/1099-lookback

#1099: Count phases as unsigned and avoid mixed arithmetic/comparisons
DARMA-tasking · Sep 30, 2020 · bee61ce · bee61ce
2 parents 4d38d23 + 3a48e8a
commit bee61ce
Show file tree

Hide file tree

Showing 22 changed files with 46 additions and 46 deletions.
diff --git a/src/vt/vrt/collection/balance/model/composed_model.cc b/src/vt/vrt/collection/balance/model/composed_model.cc
@@ -60,7 +60,7 @@ TimeType ComposedModel::getWork(ElementIDType object, PhaseOffset when) {
   return base_->getWork(object, when);
 }
 
-int ComposedModel::getNumPastPhasesNeeded(int look_back)
+unsigned int ComposedModel::getNumPastPhasesNeeded(unsigned int look_back)
 {
   return base_->getNumPastPhasesNeeded(look_back);
 }
@@ -77,7 +77,7 @@ int ComposedModel::getNumObjects() {
   return base_->getNumObjects();
 }
 
-int ComposedModel::getNumCompletedPhases() {
+unsigned int ComposedModel::getNumCompletedPhases() {
   return base_->getNumCompletedPhases();
 }
 

diff --git a/src/vt/vrt/collection/balance/model/composed_model.h b/src/vt/vrt/collection/balance/model/composed_model.h
@@ -72,13 +72,13 @@ class ComposedModel : public LoadModel
   void updateLoads(PhaseType last_completed_phase) override;
 
   TimeType getWork(ElementIDType object, PhaseOffset when) override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 
   ObjectIterator begin() override;
   ObjectIterator end() override;
 
   int getNumObjects() override;
-  int getNumCompletedPhases() override;
+  unsigned int getNumCompletedPhases() override;
   int getNumSubphases() override;
 
 private:

diff --git a/src/vt/vrt/collection/balance/model/linear_model.cc b/src/vt/vrt/collection/balance/model/linear_model.cc
@@ -61,7 +61,7 @@ TimeType LinearModel::getWork(ElementIDType object, PhaseOffset when) {
 
   PhaseOffset past_phase{when};
 
-  int phases = std::min(past_len_, getNumCompletedPhases());
+  unsigned int phases = std::min(past_len_, getNumCompletedPhases());
   // Number values on X-axis based on a PhaseOffset
   for (int i = -1 * phases; i < 0; i++) {
     x.emplace_back(i);
@@ -74,7 +74,7 @@ TimeType LinearModel::getWork(ElementIDType object, PhaseOffset when) {
   return regression.predict(when.phases);
 }
 
-int LinearModel::getNumPastPhasesNeeded(int look_back)
+unsigned int LinearModel::getNumPastPhasesNeeded(unsigned int look_back)
 {
   return ComposedModel::getNumPastPhasesNeeded(std::max(past_len_, look_back));
 }

diff --git a/src/vt/vrt/collection/balance/model/linear_model.h b/src/vt/vrt/collection/balance/model/linear_model.h
@@ -56,7 +56,7 @@ namespace vt { namespace vrt { namespace collection { namespace balance {
  */
 struct LinearModel : ComposedModel {
 
-  static constexpr int default_past_len = 5;
+  static constexpr unsigned int default_past_len = 5;
 
   /**
    * \brief Construct a linear model predictor
@@ -65,16 +65,16 @@ struct LinearModel : ComposedModel {
    * \param[in] in_past_len (optional) the past number of phases for prediction
    */
   explicit LinearModel(
-    std::shared_ptr<balance::LoadModel> base, int in_past_len = default_past_len
+    std::shared_ptr<balance::LoadModel> base, unsigned int in_past_len = default_past_len
   ) : ComposedModel(base),
       past_len_(in_past_len)
   { }
 
   TimeType getWork(ElementIDType object, PhaseOffset when) override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 
 private:
-  int past_len_ = 0;
+  const unsigned int past_len_ = 0;
 };
 
 }}}} /* end namespace vt::vrt::collection::balance */

diff --git a/src/vt/vrt/collection/balance/model/load_model.h b/src/vt/vrt/collection/balance/model/load_model.h
@@ -146,7 +146,7 @@ class LoadModel
    * \return How many phases of past load statistics will be needed to
    * satisfy the requested history
    */
-  virtual int getNumPastPhasesNeeded(int look_back = 0) = 0;
+  virtual unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) = 0;
 
   /**
    * Object enumeration, to abstract away access to the underlying structures from NodeStats
@@ -177,7 +177,7 @@ class LoadModel
    * The `updateLoads` method must have been called before any call to
    * this.
    */
-  virtual int getNumCompletedPhases() = 0;
+  virtual unsigned int getNumCompletedPhases() = 0;
 
   /**
    * Returns the number of subphases recorded in the most recent

diff --git a/src/vt/vrt/collection/balance/model/naive_persistence.cc b/src/vt/vrt/collection/balance/model/naive_persistence.cc
@@ -59,9 +59,9 @@ TimeType NaivePersistence::getWork(ElementIDType object, PhaseOffset offset)
   return ComposedModel::getWork(object, offset);
 }
 
-int NaivePersistence::getNumPastPhasesNeeded(int look_back)
+unsigned int NaivePersistence::getNumPastPhasesNeeded(unsigned int look_back)
 {
-  return ComposedModel::getNumPastPhasesNeeded(std::max(1, look_back));
+  return ComposedModel::getNumPastPhasesNeeded(std::max(1u, look_back));
 }
 
 }}}}
diff --git a/src/vt/vrt/collection/balance/model/naive_persistence.h b/src/vt/vrt/collection/balance/model/naive_persistence.h
@@ -62,7 +62,7 @@ struct NaivePersistence : public ComposedModel {
    */
   explicit NaivePersistence(std::shared_ptr<balance::LoadModel> base);
   TimeType getWork(ElementIDType object, PhaseOffset when) override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 }; // class NaivePersistence
 
 }}}} // end namespace

diff --git a/src/vt/vrt/collection/balance/model/per_collection.cc b/src/vt/vrt/collection/balance/model/per_collection.cc
@@ -80,9 +80,9 @@ TimeType PerCollection::getWork(ElementIDType object, PhaseOffset when) {
   return ComposedModel::getWork(object, when);
 }
 
-int PerCollection::getNumPastPhasesNeeded(int look_back)
+unsigned int PerCollection::getNumPastPhasesNeeded(unsigned int look_back)
 {
-  int needed = ComposedModel::getNumPastPhasesNeeded(look_back);
+  unsigned int needed = ComposedModel::getNumPastPhasesNeeded(look_back);
   for (auto& m : models_)
     needed = std::max(needed, m.second->getNumPastPhasesNeeded(look_back));
   return needed;

diff --git a/src/vt/vrt/collection/balance/model/per_collection.h b/src/vt/vrt/collection/balance/model/per_collection.h
@@ -81,7 +81,7 @@ struct PerCollection : public ComposedModel
   void updateLoads(PhaseType last_completed_phase) override;
 
   TimeType getWork(ElementIDType object, PhaseOffset when) override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 
 private:
   std::unordered_map<CollectionID, std::shared_ptr<LoadModel>> models_;

diff --git a/src/vt/vrt/collection/balance/model/persistence_median_last_n.cc b/src/vt/vrt/collection/balance/model/persistence_median_last_n.cc
@@ -48,7 +48,7 @@
 
 namespace vt { namespace vrt { namespace collection { namespace balance {
 
-PersistenceMedianLastN::PersistenceMedianLastN(std::shared_ptr<LoadModel> base, int n)
+PersistenceMedianLastN::PersistenceMedianLastN(std::shared_ptr<LoadModel> base, unsigned int n)
   : ComposedModel(base)
   , n_(n)
 {
@@ -61,10 +61,10 @@ TimeType PersistenceMedianLastN::getWork(ElementIDType object, PhaseOffset when)
   if (when.phases < 0)
     return ComposedModel::getWork(object, when);
 
-  int phases = std::min(n_, getNumCompletedPhases());
+  unsigned int phases = std::min(n_, getNumCompletedPhases());
   std::vector<TimeType> times(phases);
-  for (int i = 1; i <= phases; ++i) {
-    PhaseOffset p{-1*i, when.subphase};
+  for (unsigned int i = 1; i <= phases; ++i) {
+    PhaseOffset p{-1*static_cast<int>(i), when.subphase};
     TimeType t = ComposedModel::getWork(object, p);
     times[i-1] = t;
   }
@@ -77,7 +77,7 @@ TimeType PersistenceMedianLastN::getWork(ElementIDType object, PhaseOffset when)
     return (times[phases / 2 - 1] + times[phases / 2]) / 2;
 }
 
-int PersistenceMedianLastN::getNumPastPhasesNeeded(int look_back)
+unsigned int PersistenceMedianLastN::getNumPastPhasesNeeded(unsigned int look_back)
 {
   return ComposedModel::getNumPastPhasesNeeded(std::max(n_, look_back));
 }

diff --git a/src/vt/vrt/collection/balance/model/persistence_median_last_n.h b/src/vt/vrt/collection/balance/model/persistence_median_last_n.h
@@ -64,13 +64,13 @@ struct PersistenceMedianLastN : public ComposedModel
    * \param[in] base The underlying model
    * \param[in] n the number of preceding phases to use in making a prediction
    */
-  PersistenceMedianLastN(std::shared_ptr<LoadModel> base, int n);
+  PersistenceMedianLastN(std::shared_ptr<LoadModel> base, unsigned int n);
 
   TimeType getWork(ElementIDType object, PhaseOffset when) override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 
 private:
-  const int n_;
+  const unsigned int n_;
 }; // class PersistenceMedianLastN
 
 }}}} // namespaces

diff --git a/src/vt/vrt/collection/balance/model/raw_data.cc b/src/vt/vrt/collection/balance/model/raw_data.cc
@@ -72,7 +72,7 @@ int RawData::getNumObjects() {
   return end() - begin();
 }
 
-int RawData::getNumCompletedPhases() {
+unsigned int RawData::getNumCompletedPhases() {
   return last_completed_phase_ + 1;
 }
 
@@ -100,7 +100,7 @@ TimeType RawData::getWork(ElementIDType object, PhaseOffset offset)
   }
 }
 
-int RawData::getNumPastPhasesNeeded(int look_back)
+unsigned int RawData::getNumPastPhasesNeeded(unsigned int look_back)
 {
   return look_back;
 }

diff --git a/src/vt/vrt/collection/balance/model/raw_data.h b/src/vt/vrt/collection/balance/model/raw_data.h
@@ -70,9 +70,9 @@ struct RawData : public LoadModel {
   ObjectIterator end() override;
 
   int getNumObjects() override;
-  int getNumCompletedPhases() override;
+  unsigned int getNumCompletedPhases() override;
   int getNumSubphases() override;
-  int getNumPastPhasesNeeded(int look_back) override;
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back) override;
 
   // Observer pointers to the underlying data. In operation, these would be owned by NodeStats
   std::unordered_map<PhaseType, LoadMapType>         const* proc_load_;

diff --git a/src/vt/vrt/collection/balance/node_stats.cc b/src/vt/vrt/collection/balance/node_stats.cc
@@ -127,7 +127,7 @@ void NodeStats::clearStats() {
   next_elm_ = 1;
 }
 
-void NodeStats::startIterCleanup(PhaseType phase, int look_back) {
+void NodeStats::startIterCleanup(PhaseType phase, unsigned int look_back) {
   // TODO: Add in subphase support here too
 
   // Convert the temp ID node_data_ for the last iteration into perm ID for

diff --git a/src/vt/vrt/collection/balance/node_stats.h b/src/vt/vrt/collection/balance/node_stats.h
@@ -125,7 +125,7 @@ struct NodeStats : runtime::component::Component<NodeStats> {
   /**
    * \internal \brief Cleanup after LB runs; convert temporary to permanent IDs
    */
-  void startIterCleanup(PhaseType phase, int look_back);
+  void startIterCleanup(PhaseType phase, unsigned int look_back);
 
   /**
    * \internal \brief Release collection after LB runs for this phase

diff --git a/tests/unit/collection/test_model_comm_overhead.nompi.cc b/tests/unit/collection/test_model_comm_overhead.nompi.cc
@@ -104,12 +104,12 @@ struct StubModel : LoadModel {
     return ObjectIterator(proc_load_->at(0).end());
   }
 
-  int getNumCompletedPhases() override { return num_phases; }
+  unsigned int getNumCompletedPhases() override { return num_phases; }
 
   // Not used in this test
   int getNumObjects() override { return 0; }
   int getNumSubphases() override { return 0; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   ProcLoadMap const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_linear_model.nompi.cc b/tests/unit/collection/test_model_linear_model.nompi.cc
@@ -94,9 +94,9 @@ struct StubModel : LoadModel {
   }
 
   virtual int getNumObjects() override { return 2; }
-  virtual int getNumCompletedPhases() override { return num_phases; }
+  virtual unsigned int getNumCompletedPhases() override { return num_phases; }
   virtual int getNumSubphases() override { return 1; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   std::unordered_map<PhaseType, LoadMapType> const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_multiple_phases.nompi.cc b/tests/unit/collection/test_model_multiple_phases.nompi.cc
@@ -94,9 +94,9 @@ struct StubModel : LoadModel {
 
   // Not used by this test
   virtual int getNumObjects() override { return 0; }
-  virtual int getNumCompletedPhases() override { return 0; }
+  virtual unsigned int getNumCompletedPhases() override { return 0; }
   virtual int getNumSubphases() override { return 0; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   std::unordered_map<PhaseType, LoadMapType> const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_naive_persistence.nompi.cc b/tests/unit/collection/test_model_naive_persistence.nompi.cc
@@ -97,9 +97,9 @@ struct StubModel : LoadModel {
 
   // Not used in this test
   virtual int getNumObjects() override { return 1; }
-  virtual int getNumCompletedPhases() override { return 1; }
+  virtual unsigned int getNumCompletedPhases() override { return 1; }
   virtual int getNumSubphases() override { return 1; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   std::unordered_map<PhaseType, LoadMapType> const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_norm.nompi.cc b/tests/unit/collection/test_model_norm.nompi.cc
@@ -101,8 +101,8 @@ struct StubModel : LoadModel {
 
   // Not used in this test
   int getNumObjects() override { return 0; }
-  int getNumCompletedPhases() override { return 0; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumCompletedPhases() override { return 0; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   ProcLoadMap const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_persistence_median_last_n.nompi.cc b/tests/unit/collection/test_model_persistence_median_last_n.nompi.cc
@@ -94,9 +94,9 @@ struct StubModel : LoadModel {
   }
 
   virtual int getNumObjects() override { return 2; }
-  virtual int getNumCompletedPhases() override { return num_phases; }
+  virtual unsigned int getNumCompletedPhases() override { return num_phases; }
   virtual int getNumSubphases() override { return 1; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   std::unordered_map<PhaseType, LoadMapType> const* proc_load_ = nullptr;

diff --git a/tests/unit/collection/test_model_select_subphases.nompi.cc b/tests/unit/collection/test_model_select_subphases.nompi.cc
@@ -100,8 +100,8 @@ struct StubModel : LoadModel {
 
   // Not used in this test
   int getNumObjects() override { return 0; }
-  int getNumCompletedPhases() override { return 0; }
-  int getNumPastPhasesNeeded(int look_back = 0) override { return look_back; }
+  unsigned int getNumCompletedPhases() override { return 0; }
+  unsigned int getNumPastPhasesNeeded(unsigned int look_back = 0) override { return look_back; }
 
 private:
   ProcLoadMap const* proc_load_ = nullptr;