Honor specialFastColdContainer field in SingleChainFastMoveType

Summary:
`SINGLE_CHAIN_FAST` was ignoring its `specialFastColdContainer` parameter. Even with a fixed cold container configured, the hot object could still move to any container.

Root cause is that `findBestMove()` built its own cold-container set from all containers and never called `getCustomColdContainers()`, the base-class function that applies the special cold container.

We fix this by seeding the cold set from `getCustomColdContainers()`, the same way the base type does. We also forward the fast spec's partition name and `specialFastColdContainer` into the base `SingleChainMoveTypeSpec`, so the fast type reuses the base machinery and no longer needs its own members or overrides.

`specialFastColdContainer` is not being used by any use case currently and this is likely we didn't see this bug before.

Reviewed By: polmauri

Differential Revision: D112341411

fbshipit-source-id: 53f74636e684de15e351960247a0ee1e6f4fb540

Author: vj-menon

algopt/rebalancer/solver/moves/SingleChainFastMoveType.cpp

@@ -21,18 +21,25 @@
 
 namespace facebook::rebalancer {
 
+namespace {
+interface::SingleChainMoveTypeSpec toSingleChainMoveTypeSpec(
+    const interface::SingleChainFastMoveTypeSpec& spec) {
+  interface::SingleChainMoveTypeSpec baseSpec;
+  baseSpec.partitionNameToExploreChainsWithinObjectGroup().copy_from(
+      spec.partitionNameToExploreFastChainsWithinObjectGroup());
+  baseSpec.specialColdContainer().copy_from(spec.specialFastColdContainer());
+  return baseSpec;
+}
+} // namespace
+
 std::string SingleChainFastMoveType::name() const {
   return kSingleChainFastMoveTypeName.str();
 }
 
 SingleChainFastMoveType::SingleChainFastMoveType(
     const interface::LocalSearchSolverSpec& solverConfigs,
     const interface::SingleChainFastMoveTypeSpec& spec)
-    : SingleChainMoveType(solverConfigs, interface::SingleChainMoveTypeSpec()) {
-  partitionNameToExploreFastChainsWithinObjectGroup_ =
-      spec.partitionNameToExploreFastChainsWithinObjectGroup().to_optional();
-  specialFastColdContainer_ = spec.specialFastColdContainer().to_optional();
-}
+    : SingleChainMoveType(solverConfigs, toSingleChainMoveTypeSpec(spec)) {}
 
 MoveResult SingleChainFastMoveType::findBestMove(
     const MovesEvaluator& evaluator,
@@ -45,8 +52,10 @@ MoveResult SingleChainFastMoveType::findBestMove(
   const ObjectDeduper dedupedObjs(
       &problem.getEquivalenceSets(), dynamicObjects);
 
+  const auto customColdContainers =
+      getCustomColdContainers(evaluator, hotContainer);
   auto coldContainers = Filter(
-      problem.containers,
+      customColdContainers ? *customColdContainers : problem.containers,
       [&problem, hotContainer](entities::ContainerId container) {
         return container != hotContainer &&
             !problem.not_accepting_containers.contains(container);

algopt/rebalancer/solver/moves/SingleChainFastMoveType.h

@@ -51,20 +51,6 @@ class SingleChainFastMoveType : public SingleChainMoveType {
       MoveStatsAggregator& stats,
       const SearchHints& hints,
       double timeLimit) override;
-
- protected:
-  std::optional<std::string> getPartitionNameToExploreChainsWithinObjectGroup()
-      const override {
-    return partitionNameToExploreFastChainsWithinObjectGroup_;
-  }
-  std::optional<std::string> getSpecialColdContainer() const override {
-    return specialFastColdContainer_;
-  }
-
- private:
-  std::optional<std::string>
-      partitionNameToExploreFastChainsWithinObjectGroup_ = std::nullopt;
-  std::optional<std::string> specialFastColdContainer_ = std::nullopt;
 };
 
 } // namespace facebook::rebalancer

algopt/rebalancer/solver/moves/tests/SingleChainFastMoveTypeTest.cpp

@@ -23,6 +23,8 @@
 #include <folly/coro/BlockingWait.h>
 #include <gtest/gtest.h>
 
+#include <limits>
+
 namespace facebook::rebalancer::packer::tests {
 
 class MockSingleChainFastMoveType : public SingleChainFastMoveType {
@@ -291,6 +293,43 @@ CO_TEST_F(SingleChainFastMoveTypeTest, TestTotalNumberOfEvaluations) {
   EXPECT_EQ(8, getTotalMovesEvaluated());
 }
 
+// specialFastColdContainer should send the hot object only to that cold
+// container, so setting it should evaluate fewer moves. The const-0 objective
+// makes all objects equal, so no move improves (nothing is skipped early) and
+// each container counts as one object. A chain is 2 moves: the hot object goes
+// to a cold container, and one object from another container moves into the hot
+// container.
+CO_TEST_F(
+    SingleChainFastMoveTypeTest,
+    FindBestMoveHonorsSpecialFastColdContainer) {
+  const auto universe = co_await setUpUniverse();
+  createProblem({const_expr(0, *universe)}, const_expr(0, *universe));
+
+  const auto movesEvaluated =
+      [&](const interface::SingleChainFastMoveTypeSpec& spec) {
+        MockSingleChainFastMoveType(interface::LocalSearchSolverSpec{}, spec)
+            .findBestMove(
+                getMovesEvaluator(),
+                container(1) /*hotContainer*/,
+                getMoveStatsAggregator(),
+                getEmptySearchHints(),
+                std::numeric_limits<double>::max() /*timeLimit*/);
+        return getTotalMovesEvaluated();
+      };
+
+  interface::SingleChainFastMoveTypeSpec restricted;
+  restricted.specialFastColdContainer() = "region2";
+
+  // Cold containers {region2, region3, region4}; region2 is empty:
+  //   into region2: from region3 and region4 = 2 chains = 4 moves
+  //   into region3: from region4             = 1 chain  = 2 moves
+  //   into region4: from region3             = 1 chain  = 2 moves
+  EXPECT_EQ(8, movesEvaluated(interface::SingleChainFastMoveTypeSpec{}));
+
+  // Restricting to region2 keeps only the first line: 4 moves.
+  EXPECT_EQ(4, movesEvaluated(restricted));
+}
+
 TEST_F(SingleChainFastMoveTypeTest, Name) {
   auto mockSingleChainFastMoveType = MockSingleChainFastMoveType(
       interface::LocalSearchSolverSpec{},