Merge pull request #10431 from erendn/resizer_nested_journal_fix

rsz: Support nested journals' move result tracking

Author: maliberty

src/rsz/src/MoveCommitter.cc

@@ -8,6 +8,7 @@
 #include <cassert>
 #include <cstddef>
 #include <memory>
+#include <stack>
 #include <string>
 #include <unordered_set>
 #include <utility>
@@ -59,15 +60,15 @@ MoveCommitter::MoveCommitter(Resizer& resizer) : resizer_(resizer)
 
 void MoveCommitter::init()
 {
-  journal_open_ = false;
+  pending_move_results_ = std::stack<std::vector<MoveResult>>{};
   std::ranges::fill(summary_carried_by_type_, 0);
   summary_carried_by_type_[typeIndex(MoveType::kSizeUpMatch)]
       = persisted_summary_by_type_[typeIndex(MoveType::kSizeUpMatch)];
   std::ranges::fill(pending_by_type_, 0);
   std::ranges::fill(committed_by_type_, 0);
   for (size_t index = 0; index < pending_instances_by_type_.size(); ++index) {
     pending_instances_by_type_[index].clear();
-    all_instances_by_type_[index].clear();
+    committed_instances_by_type_[index].clear();
   }
 
   const bool report_enabled
@@ -90,6 +91,12 @@ MoveResult MoveCommitter::commit(MoveCandidate& candidate)
   MoveResult result = candidate.apply();
   if (result.accepted) {
     recordAcceptedResult(result);
+    // Only the journaled path needs the per-level result trail; non-journaled
+    // commits (e.g. MeasuredVtSwapPolicy) flush via acceptPendingMoves and
+    // never restore.
+    if (!pending_move_results_.empty()) {
+      pending_move_results_.top().push_back(result);
+    }
   }
   return result;
 }
@@ -366,16 +373,28 @@ bool MoveCommitter::canManageJournal() const
 
 void MoveCommitter::recordAcceptedResult(const MoveResult& result)
 {
-  // Track committed instances by move type so later passes can avoid
-  // conflicts.
+  // Track committed instances by move type so later passes can avoid conflicts
   const size_t index = typeIndex(result.type);
   pending_by_type_[index] += result.move_count;
   for (sta::Instance* inst : result.touched_instances) {
     pending_instances_by_type_[index].insert(inst);
     // Preserve legacy BaseMove semantics: once a move type touched an
     // instance in this repair session, later guards still see it even if the
     // journal branch is restored.
-    all_instances_by_type_[index].insert(inst);
+    // FIXME: This should be removed after policies start using the nested
+    // journaling capability.
+    committed_instances_by_type_[index].insert(inst);
+  }
+}
+
+void MoveCommitter::unrecordAcceptedResult(const MoveResult& result)
+{
+  // Remove the reverted move results
+  const size_t index = typeIndex(result.type);
+  pending_by_type_[index] -= result.move_count;
+  for (sta::Instance* inst : result.touched_instances) {
+    pending_instances_by_type_[index].erase(
+        pending_instances_by_type_[index].find(inst));
   }
 }
 
@@ -392,16 +411,13 @@ void MoveCommitter::beginEcoJournal() const
 
 void MoveCommitter::commitEcoJournal() const
 {
-  debugPrint(resizer_.logger(), RSZ, "journal", 1, "journal end");
-  // Refresh timing only once after a batch of ECO edits becomes permanent.
-  if (ecoHasPendingChanges()) {
-    resizer_.updateParasiticsAndTiming();
-  }
+  debugPrint(resizer_.logger(), RSZ, "journal", 1, "journal commit");
   odb::dbDatabase::commitEco(resizer_.block());
 }
 
 void MoveCommitter::restoreEcoJournal() const
 {
+  debugPrint(resizer_.logger(), RSZ, "journal", 1, "journal restore");
   odb::dbDatabase::undoEco(resizer_.block());
 }
 
@@ -466,51 +482,115 @@ void MoveCommitter::beginJournal()
   if (!canManageJournal()) {
     return;
   }
-
-  // Start one reversible ECO batch for the current search branch.
+  // Start one reversible ECO batch for the current search branch. Stacks
+  // naturally: odb's beginEco pushes the current journal aside, and we mirror
+  // that with a fresh per-level result vector here.
   beginEcoJournal();
-  journal_open_ = true;
+  pending_move_results_.emplace();
+  // Mirror the journal level in the tracker so a nested restore rejects only
+  // its own moves.
+  if (move_tracker_) {
+    move_tracker_->beginJournal();
+  }
 }
 
 void MoveCommitter::commitJournal()
 {
-  if (!canManageJournal() || !journal_open_) {
+  if (!canManageJournal() || pending_move_results_.empty()) {
     return;
   }
-
-  // Close the reversible batch and fold the accepted moves into the totals.
+  // Refresh timing only once after a batch of ECO edits becomes permanent
+  if (ecoHasPendingChanges()) {
+    resizer_.updateParasiticsAndTiming();
+  }
+  // Close the reversible batch. odb::commitEco appends the inner journal to its
+  // parent when one exists, otherwise the changes become permanent.
   commitEcoJournal();
-  acceptPendingMoves();
-  journal_open_ = false;
+  // Mirror the journal commit in the tracker: an outermost commit finalizes
+  // the tracked moves, a nested commit merges them into the parent level.
+  if (move_tracker_) {
+    move_tracker_->commitJournal();
+  }
+
+  std::vector<MoveResult> top = std::move(pending_move_results_.top());
+  pending_move_results_.pop();
+
+  if (pending_move_results_.empty()) {
+    // Outermost commit: fold the accumulated pending counters into the
+    // committed totals and flush the tracker.
+    acceptPendingMoves();
+  } else {
+    // Nested commit: the moves remain pending against the parent journal,
+    // so the parent restore can still undo them. Merge our results into
+    // the parent's vector; pending_by_type_ / pending_instances_by_type_
+    // already reflect the cumulative state and stay untouched.
+    debugPrint(resizer_.logger(),
+               RSZ,
+               "journal",
+               2,
+               "nested journal commit: merged {} results into parent",
+               top.size());
+    auto& parent = pending_move_results_.top();
+    for (MoveResult& result : top) {
+      parent.push_back(std::move(result));
+    }
+  }
 }
 
 void MoveCommitter::restoreJournal()
 {
-  if (!canManageJournal() || !journal_open_) {
+  if (!canManageJournal() || pending_move_results_.empty()) {
     return;
   }
-
-  // Undo both database edits and in-memory accounting for the rejected branch.
+  // Undo both database edits and in-memory accounting for the rejected branch
   debugPrint(
       resizer_.logger(), RSZ, "journal", 1, "journal restore starts >>>");
   resizer_.initForJournalRestore();
 
   const bool had_eco_changes = ecoHasPendingChanges();
   restoreEcoJournal();
-  if (!had_eco_changes) {
-    rejectPendingMoves();
-    journal_open_ = false;
+  // Mirror the journal restore in the tracker so its moves are rejected,
+  // whether this is an outermost or a nested restore.
+  if (move_tracker_) {
+    move_tracker_->restoreJournal();
+  }
+
+  // Unrecord just this level's accepted results; the parent's accumulated
+  // pending state (if any) must survive.
+  std::vector<MoveResult> top = std::move(pending_move_results_.top());
+  pending_move_results_.pop();
+  int reverted_moves = 0;
+  for (const MoveResult& result : top) {
+    reverted_moves += result.move_count;
+  }
+  resizer_.addRejectedLegacyMoveCount(reverted_moves);
+  if (pending_move_results_.empty()) {
+    // pending_by_type_ still holds the about-to-be-reverted counts at this
+    // point; log the per-type breakdown before draining them.
+    logPendingMoves("UNDO", reverted_moves);
+  } else {
     debugPrint(resizer_.logger(),
                RSZ,
                "journal",
-               1,
-               "journal restore ends due to empty ECO >>>");
-    return;
+               2,
+               "nested journal restore: reverted {} moves",
+               reverted_moves);
+  }
+  for (const MoveResult& result : top) {
+    unrecordAcceptedResult(result);
   }
 
-  resizer_.updateParasiticsAndTiming();
-  rejectPendingMoves();
-  journal_open_ = false;
+  if (had_eco_changes) {
+    resizer_.updateParasiticsAndTiming();
+    resizer_.invalidateVertexOrdering();
+  }
+
+  if (pending_move_results_.empty()) {
+    // Outermost restore: the tracker's pending entries are no longer
+    // load-bearing. Flush them and emit the totals snapshot.
+    rejectTrackedMoves();
+    logCommittedTotals();
+  }
 
   debugPrint(resizer_.logger(), RSZ, "journal", 1, "journal restore ends <<<");
 }
@@ -527,7 +607,7 @@ void MoveCommitter::acceptPendingMoves()
     }
     committed_by_type_[index] += pending_by_type_[index];
     pending_by_type_[index] = 0;
-    all_instances_by_type_[index].insert(
+    committed_instances_by_type_[index].insert(
         pending_instances_by_type_[index].begin(),
         pending_instances_by_type_[index].end());
     pending_instances_by_type_[index].clear();
@@ -544,7 +624,7 @@ void MoveCommitter::rejectPendingMoves()
   resizer_.addRejectedLegacyMoveCount(move_count);
 
   std::ranges::fill(pending_by_type_, 0);
-  for (std::unordered_set<sta::Instance*>& pending_instances :
+  for (std::unordered_multiset<sta::Instance*>& pending_instances :
        pending_instances_by_type_) {
     pending_instances.clear();
   }
@@ -585,7 +665,7 @@ bool MoveCommitter::hasPendingMoves(const MoveType type,
 bool MoveCommitter::hasMoves(const MoveType type, sta::Instance* inst) const
 {
   const size_t index = typeIndex(type);
-  return all_instances_by_type_[index].contains(inst)
+  return committed_instances_by_type_[index].contains(inst)
          || pending_instances_by_type_[index].contains(inst);
 }
 

src/rsz/src/MoveCommitter.hh

@@ -6,6 +6,7 @@
 #include <array>
 #include <cstddef>
 #include <memory>
+#include <stack>
 #include <string>
 #include <unordered_set>
 #include <utility>
@@ -137,6 +138,7 @@ class MoveCommitter
   // === Commit result handling ==============================================
   bool canManageJournal() const;
   void recordAcceptedResult(const MoveResult& result);
+  void unrecordAcceptedResult(const MoveResult& result);
   int pendingMoveCount() const;
 
   // === ECO journal internals ===============================================
@@ -156,14 +158,18 @@ class MoveCommitter
   Resizer& resizer_;
 
   // === Journal and move accounting state ===================================
-  bool journal_open_{false};
+  // Stack depth equals the number of currently open ECO journals. Each vector
+  // holds the results accepted at that nesting level so a nested restore can
+  // unrecord just its own moves and a nested commit can merge its results into
+  // the parent's vector (mirroring odb's commitEco append-to-parent semantics).
+  std::stack<std::vector<MoveResult>> pending_move_results_;
   std::array<int, kTypeCount> pending_by_type_{};
   std::array<int, kTypeCount> committed_by_type_{};
   std::array<int, kTypeCount> summary_carried_by_type_{};
-  std::array<std::unordered_set<sta::Instance*>, kTypeCount>
+  std::array<std::unordered_multiset<sta::Instance*>, kTypeCount>
       pending_instances_by_type_{};
   std::array<std::unordered_set<sta::Instance*>, kTypeCount>
-      all_instances_by_type_{};
+      committed_instances_by_type_{};
 
   // === MoveTracker state ====================================================
   std::unique_ptr<MoveTracker> move_tracker_;