✨ Implement mark-and-sweep garbage collection #980

q-inho · 2025-06-02T01:37:07Z

Description

The DD package now uses a mark-and-sweep garbage collector instead of reference counting.
Nodes no longer store reference counts.
The incRef and decRef functions now manage the root set for the new garbage collector.

Resolves #644

…nagement in DD package

…r improved performance

q-inho · 2025-06-02T05:38:57Z

@burgholzer , Do you have any tips for testing for the failed test? I locally test using these commands

cmake -S . -B build -DCMAKE_BUILD_TYPE=Release
cmake --build build --config Release
ctest -C Release --test-dir build/test/dd

but it shows follwong on my test


Start 120: DDPackageTest.ResetClearsRoots
120/288 Test #120: DDPackageTest.ResetClearsRoots ................................................................................   Passed    0.00 sec

burgholzer

Hey 👋🏻
Many thanks for the changes here. This is really appreciated!

I would not consider this a full review yet. But I wanted to get a first batch of comments out to you. Take everything with a grain of salt.
As I was reading through the PR, my opinion on how this should probably look like evolved a bit. So some of the comments might not perfectly align with each other.
Generally, I believe the high-level incRef/decRef (or track/untrack) functions are here to stay. The key difference is that the actual reference counts are now only evaluated once the unique tables are full enough to trigger collection instead of all the time during computation.

Feel free to ask any questions that pop up. You can see the clang-tidy linter complaints on the summary section on the GitHub Actions summary page.

include/mqt-core/dd/Node.hpp

include/mqt-core/dd/Package.hpp

burgholzer · 2025-06-02T07:06:49Z

include/mqt-core/dd/Package.hpp

+  // helper to track complex number references recursively
+  template <class Node> void markComplexRec(Node* p, bool inc) noexcept {
+    if (Node::isTerminal(p)) {
+      return;
+    }
+    for (const auto& child : p->e) {
+      if (inc) {
+        cn.incRef(child.w);
+      } else {
+        cn.decRef(child.w);
      }
+      markComplexRec(child.p, inc);
    }
  }


As per the other comment, I believe to would be best to handle this differently. Specifically, to handle numbers similarly to the nodes here.
Note that the RealNumber class does not have any space for flags.
So you probably have to rely on a similar trick to the one we use for marking negative numbers, manipulating the lower bits of the pointer to the numbers.
A 1 in the LSB of the pointer address indicates the number is negative.
One could use the second bit to mark entries for example.

burgholzer · 2025-06-02T07:14:34Z

include/mqt-core/dd/statistics/UniqueTableStatistics.hpp

+   * @brief The total number of active entries.
   */
  std::size_t numActiveEntries = 0U;


Active entries do not make any sense any more now that they are not being tracked through continuous reference counting anymore.
So any mentions of it can essentially be removed.

It could be helpful to provide a method in the DD package that still allows to compute this by marking all active entries (using the new mark method) and then counting in the unique table.

burgholzer · 2025-06-02T07:24:57Z

src/dd/Package.cpp

+  // Collecting complex numbers forces collection of the node tables. The
+  // following calls explicitly use `true` to enforce this.
+  const auto vCollect = vUniqueTable.garbageCollect(true);
+  const auto mCollect = mUniqueTable.garbageCollect(true);
+  const auto dCollect = dUniqueTable.garbageCollect(true);


force should only be set to true here if complex numbers have been collected. Similar to the original code.

burgholzer · 2025-06-02T07:34:58Z

src/dd/Simulation.cpp

@@ -120,7 +120,7 @@ std::map<std::string, std::size_t> sample(const qc::QuantumComputation& qc,
      auto measurement = dd.measureAll(e, false, mt);
      counts.operator[](measurement) += 1U;
    }
-    // reduce reference count of measured state
+    // remove the measured state edge from the root set
    dd.decRef(e);


Just based on the changes to the docstrings throughout the files I believe these methods deserve a more appropriate name now. Maybe something like track and untrack?

The rename should also help to revisit all the places where these functions are called, to see whether these calls are still necessary. One of the goals here would actually be to require as little as possible manual intervention.
The garbage collection would ideally check in periodic intervals how full the unique tables are. If one of them could be collected, the marking and collection would start.

src/mqt/core/dd.pyi

test/dd/test_package.cpp

q-inho · 2025-06-02T10:04:22Z

Thank you for the detailed review. I began with a minimal core implementation, but realized later many other codes are interacted, resulting existing code paths to be escaped from my purpose. I really appreciate for your feedback.

burgholzer · 2025-06-02T10:48:17Z

@burgholzer , Do you have any tips for testing for the failed test? I locally test using these commands

cmake -S . -B build -DCMAKE_BUILD_TYPE=Release

cmake --build build --config Release

ctest -C Release --test-dir build/test/dd

but it shows follwong on my test




Start 120: DDPackageTest.ResetClearsRoots

120/288 Test #120: DDPackageTest.ResetClearsRoots ................................................................................   Passed    0.00 sec

As for testing, the CI is simply running (a version of)

ctest -C Release --test-dir build/ --output-on-failure

So that should allow you to see the same results.

If you use an IDE like CLion (which ai can only recommend. And CLion is free since a couple weeks ago), then you'll also get an "All tests" target that you can execute directly from the IDE.

q-inho · 2025-06-02T10:51:36Z

@burgholzer , Do you have any tips for testing for the failed test? I locally test using these commands
cmake -S . -B build -DCMAKE_BUILD_TYPE=Release

cmake --build build --config Release

ctest -C Release --test-dir build/test/dd
but it shows follwong on my test
Start 120: DDPackageTest.ResetClearsRoots

120/288 Test #120: DDPackageTest.ResetClearsRoots ................................................................................   Passed    0.00 sec
As for testing, the CI is simply running (a version of)
ctest -C Release --test-dir build/ --output-on-failure
So that should allow you to see the same results.

If you use an IDE like CLion (which ai can only recommend. And CLion is free since a couple weeks ago), then you'll also get an "All tests" target that you can execute directly from the IDE.

I didn't know CLion is free. Thats really a good tip to know. Thanks a lot!

Co-authored-by: Lukas Burgholzer <[email protected]> Signed-off-by: Inho Choi <[email protected]>

…ounting and custom hash/equality functions for edges

codecov · 2025-06-04T11:16:07Z

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/dd/RealNumberUniqueTable.cpp	93.7%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

q-inho · 2025-06-04T12:00:59Z

@burgholzer I twisted some strategies and now it is passing the tests. May I ask for your further feedbacks?

burgholzer

Many thanks for your continued work on this. It is certainly heading in the right direction. Great progress. 🙌
I think I have now reviewed the full PR in all its detail. You can find respective comments inline.

Please try to address all of them in your next iteration or explicitly ask questions (as reply to the comments) if something is unclear.

This will still be some work, but I am sure this is doable 😌

Edit: note that there are still some unresolved/unaddressed comments from the first review. Some of them partially overlap with the comments in the latest review, but some don't.

burgholzer · 2025-06-04T12:34:15Z

include/mqt-core/dd/Edge.hpp

+namespace dd {
+template <class Node> struct EdgePtrHash {
+  std::size_t operator()(const Edge<Node>& e) const noexcept {
+    const auto h1 = murmur64(reinterpret_cast<std::size_t>(e.p));
+    const auto h2 = murmur64(reinterpret_cast<std::size_t>(e.w.r));
+    const auto h3 = murmur64(reinterpret_cast<std::size_t>(e.w.i));
+    return qc::combineHash(qc::combineHash(h1, h2), h3);
+  }
+};
+
+template <class Node> struct EdgePtrEqual {
+  bool operator()(const Edge<Node>& lhs, const Edge<Node>& rhs) const noexcept {
+    return lhs.p == rhs.p && lhs.w.r == rhs.w.r && lhs.w.i == rhs.w.i;
+  }
+};
+} // namespace dd
+


This change should not be necessary if I am not overlooking things.
The Edge class defines a hash function at the very end of this file.

And the equality operator defined for the Edge class should also be sufficient here.

You are right. Edge already provides them.

include/mqt-core/dd/Package.hpp

burgholzer · 2025-06-04T12:50:50Z

include/mqt-core/dd/Package.hpp

+  template <class Node> bool addRoot(const Edge<Node>& e) {
+    if constexpr (std::is_same_v<Node, vNode>) {
+      auto it = vectorRoots.find(e);
+      if (it == vectorRoots.end()) {
+        vectorRoots.emplace(e, 1U);
+        return true;
+      }
+      ++it->second;
+      return false;
+    } else if constexpr (std::is_same_v<Node, mNode>) {
+      auto it = matrixRoots.find(e);
+      if (it == matrixRoots.end()) {
+        matrixRoots.emplace(e, 1U);
+        return true;
+      }
+      ++it->second;
+      return false;
+    } else if constexpr (std::is_same_v<Node, dNode>) {
+      auto it = densityRoots.find(e);
+      if (it == densityRoots.end()) {
+        densityRoots.emplace(e, 1U);
+        return true;
+      }
+      ++it->second;
+      return false;
+    }
+    return false;
+  }
+
+  template <class Node> bool removeRoot(const Edge<Node>& e) {
+    if constexpr (std::is_same_v<Node, vNode>) {
+      auto it = vectorRoots.find(e);
+      if (it != vectorRoots.end()) {
+        if (--it->second == 0U) {
+          vectorRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    } else if constexpr (std::is_same_v<Node, mNode>) {
+      auto it = matrixRoots.find(e);
+      if (it != matrixRoots.end()) {
+        if (--it->second == 0U) {
+          matrixRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    } else if constexpr (std::is_same_v<Node, dNode>) {
+      auto it = densityRoots.find(e);
+      if (it != densityRoots.end()) {
+        if (--it->second == 0U) {
+          densityRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    }
+    return false;
+  }


I am fairly sure both of these functions can simply be void functions.
Their return type is not used in the main library and this would also remove any need for casts to void. In the existing incRef and decRef functions.

burgholzer · 2025-06-04T12:56:49Z

include/mqt-core/dd/Package.hpp

+      auto it = vectorRoots.find(e);
+      if (it == vectorRoots.end()) {
+        vectorRoots.emplace(e, 1U);
+        return true;
+      }
+      ++it->second;


I am not 100% sure, but if I remember correctly, this can be simplified to

vectorRoots[e]++;

I might be wrong though.

burgholzer · 2025-06-04T13:00:18Z

include/mqt-core/dd/Package.hpp

+  template <class Node> bool removeRoot(const Edge<Node>& e) {
+    if constexpr (std::is_same_v<Node, vNode>) {
+      auto it = vectorRoots.find(e);
+      if (it != vectorRoots.end()) {
+        if (--it->second == 0U) {
+          vectorRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    } else if constexpr (std::is_same_v<Node, mNode>) {
+      auto it = matrixRoots.find(e);
+      if (it != matrixRoots.end()) {
+        if (--it->second == 0U) {
+          matrixRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    } else if constexpr (std::is_same_v<Node, dNode>) {
+      auto it = densityRoots.find(e);
+      if (it != densityRoots.end()) {
+        if (--it->second == 0U) {
+          densityRoots.erase(it);
+          return true;
+        }
+        return false;
+      }
+    }
+    return false;


I believe this function should handle the case where the Edge is not in the root set and error out in that case.

burgholzer · 2025-06-04T13:26:14Z

include/mqt-core/dd/Package.hpp

+  template <class Node> void markNodes(Node* p) noexcept {
+    if (Node::isTerminal(p) || p->marked()) {
+      return;
+    }
+    p->mark();
+    for (const auto& child : p->e) {
+      cn.incRef(child.w);
+      markNodes(child.p);
    }
  }


I believe this function should be marking Edges now. Because otherwise, you will never be marking the top edge weight.

burgholzer · 2025-06-04T13:46:02Z

src/dd/Package.cpp

+  // first sweep all node tables
+  const auto vCollect = vUniqueTable.garbageCollect(true);
+  const auto mCollect = mUniqueTable.garbageCollect(true);
+  const auto dCollect = dUniqueTable.garbageCollect(true);


As pointed out in the previous review, this is not yet the same logic as before this PR with regard to the force flag.

burgholzer · 2025-06-04T13:51:06Z

src/dd/Package.cpp

+  // unmark all complex numbers again so reference counts are reset for the next
+  // collection cycle
+  {
+    std::unordered_set<vNode*> visited{};
+    for (const auto& [edge, _] : vectorRoots) {
+      cn.decRef(edge.w);
+      markComplexNumbersRec(edge.p, false, &visited);
+    }
+  }
+  {
+    std::unordered_set<mNode*> visited{};
+    for (const auto& [edge, _] : matrixRoots) {
+      cn.decRef(edge.w);
+      markComplexNumbersRec(edge.p, false, &visited);
+    }
+  }
+  {
+    std::unordered_set<dNode*> visited{};
+    for (const auto& [edge, _] : densityRoots) {
+      cn.decRef(edge.w);
+      markComplexNumbersRec(edge.p, false, &visited);
+    }


I would assume this is no longer necessary once the reference counting for the numbers is adjusted.
Maybe the top level edge-weights need to be unmarked, but no recursive traversal should be necessary.

burgholzer · 2025-06-04T13:53:19Z

src/dd/Package.cpp

@@ -163,7 +204,6 @@ dEdge Package::makeZeroDensityOperator(const std::size_t n) {
    f = makeDDNode(static_cast<Qubit>(p),
                   std::array{f, dEdge::zero(), dEdge::zero(), dEdge::zero()});
  }
-  incRef(f);


Based on where this PR is heading, this incRef needs to stay.

burgholzer · 2025-06-04T13:53:35Z

src/dd/Package.cpp

@@ -179,7 +219,6 @@ vEdge Package::makeZeroState(const std::size_t n, const std::size_t start) {
  for (std::size_t p = start; p < n + start; p++) {
    f = makeDDNode(static_cast<Qubit>(p), std::array{f, vEdge::zero()});
  }
-  incRef(f);


Based on where this PR is heading, this incRef needs to stay.

q-inho · 2025-06-04T14:10:47Z

Thanks a lot for detailed comments. I am really learning a lot from this PR about cpp projects. I will reach out for further questions if I am not clear.

Co-authored-by: Lukas Burgholzer <[email protected]> Signed-off-by: Inho Choi <[email protected]>

burgholzer · 2025-06-09T20:05:18Z

@q-inho may I ask for an update on this? Is there anything blocking your progress?

q-inho · 2025-06-09T20:27:25Z

@burgholzer Thank you for asking. I am currently not passing the test for Grover, so I am reviewing the changes I made, where it is going wrong. I am reviewing the codes myself. 😅

burgholzer · 2025-06-09T21:15:57Z

@burgholzer Thank you for asking. I am currently not passing the test for Grover, so I am reviewing the changes I made, where it is going wrong. I am reviewing the codes myself. 😅

Have you already implemented the changes to the handling of the complex number ref counts? If so, it might be worth pushing the changes even if tests fail. Maybe I can quickly point you towards the underlying error.

q-inho · 2025-06-09T22:11:30Z

@burgholzer Thank you for asking. I am currently not passing the test for Grover, so I am reviewing the changes I made, where it is going wrong. I am reviewing the codes myself. 😅

Have you already implemented the changes to the handling of the complex number ref counts? If so, it might be worth pushing the changes even if tests fail. Maybe I can quickly point you towards the underlying error.

I believed I made correct implementation on complex number ref counts. I will check little bit more and will push soon. Thank you for your help

burgholzer · 2025-06-11T10:02:29Z

@q-inho
Just fyi: today (anywhere on earth) is officially the last day of unitary hack. Since you submitted the PRs before that deadline, you can keep working on them until next Monday.
Of course, you are welcome to keep working on the pull request after that. It just won't be counted towards the unitary hack bounties.

burgholzer · 2025-06-13T12:11:19Z

@q-inho may I ask for an update on this?

…y Management

q-inho · 2025-06-13T13:07:24Z

@burgholzer Apologize for delay due to urgent works. I just updated current progress. I think this now implement mark & sweep and replaces node reference counts with a mark flag and manages roots through Package::incRef/decRef. Complex numbers are kept in RealNumberUniqueTable and marked via pointer bits.

…r clarity

…age_DD_package

… in Windows

q-inho · 2025-06-15T03:05:57Z

The Windows build failed because it requires clearMark to be declared as constexpr, while passing on Mac and Linux. However, defining clearMark as constexpr require the use of reinterpret_cast, which is not permitted in constant expressions. As a temporary workaround, I have replaced constexpr with inline.

…truct

Signed-off-by: burgholzer <[email protected]>

burgholzer · 2025-06-15T22:51:54Z

Hey @q-inho 👋🏼
could you please give me push access to your fork so that I can push some changes to this PR?

q-inho · 2025-06-15T22:57:40Z

@burgholzer Thanks for your help! I've already enabled Allow edits by maintainers for this PR. Did you mean I should grant you direct access by adding you as a collaborator on my repository branch instead?

burgholzer · 2025-06-15T23:04:42Z

Yeah, thanks!

I just pushed a work-in-progress commit with some fixups and corrections.
This is not yet functional unfortunately, but I need to get some sleep now.
I'll investigate more tomorrow. If you want to look at in the meantime, feel free to go ahead.
Something about the complex number tracking is still off. The mark and unmark does not seem to work fully.

✨ Implement mark-and-sweep garbage collection and update reference ma…

a9d5ffe

…nagement in DD package

github-project-automation bot added this to MQT Core Jun 2, 2025

q-inho and others added 2 commits June 2, 2025 13:24

✨ Refactor garbage collection root management to use unordered_set fo…

8a3646e

…r improved performance

Merge branch 'main' into mark_and_sweep_garbage_DD_package

46d7a71

burgholzer requested changes Jun 2, 2025

View reviewed changes

q-inho and others added 4 commits June 2, 2025 19:56

Update include/mqt-core/dd/Node.hpp

e03554c

Co-authored-by: Lukas Burgholzer <[email protected]> Signed-off-by: Inho Choi <[email protected]>

✨ Enhance mark-and-sweep garbage collection with explicit reference c…

73ef0ff

…ounting and custom hash/equality functions for edges

Merge branch 'main' into mark_and_sweep_garbage_DD_package

2f216ef

Fix RealNumber reference count assertion

ad64bf9

q-inho requested a review from burgholzer June 4, 2025 12:00

burgholzer requested changes Jun 4, 2025

View reviewed changes

Update include/mqt-core/dd/Package.hpp

46e7aa9

Co-authored-by: Lukas Burgholzer <[email protected]> Signed-off-by: Inho Choi <[email protected]>

burgholzer moved this to In Progress in MQT Core Jun 9, 2025

burgholzer added enhancement New feature or request minor Minor version update DD Anything related to the DD package c++ Anything related to C++ code unitaryHack Issues and PRs intended for unitaryHack labels Jun 9, 2025

burgholzer added this to the DD Package Improvements milestone Jun 9, 2025

Refactor RealNumber, ComplexNumber and UniqueTable for Improved Memor…

c88a319

…y Management

q-inho and others added 9 commits June 13, 2025 22:41

constexpr pointer operations in RealNumber

143c1db

RealNumber with mark and unmark functions

6671038

Update tests to use makeBasisState instead of makeZeroState for bette…

f5877d0

…r clarity

Revert changes

bed77c0

Merge remote-tracking branch 'upstream/main' into mark_and_sweep_garb…

4a1a121

…age_DD_package

Change the call to makeZeroState to the dd namespace

7e768b6

add bit_cast support for clearMark function in RealNumber for Build…

ce4afda

… in Windows

Merge branch 'main' into mark_and_sweep_garbage_DD_package

c51a5de

Replace constexp to inline due to build failure on Window

d75f141

q-inho and others added 3 commits June 15, 2025 17:20

Remove constexpr from exactlyZero and exactlyOne methods in Complex s…

d6dec27

…truct

Remove constexpr from isStaticComplex method in ComplexNumbers class

8344696

🚧 work-in-progress cleanup

26ba8d5

Signed-off-by: burgholzer <[email protected]>

MatthiasReumann mentioned this pull request Jun 24, 2025

🚧 Mark-And-Sweep Garbage Collection #1020

Draft

8 tasks

✨ Implement mark-and-sweep garbage collection #980

Are you sure you want to change the base?

✨ Implement mark-and-sweep garbage collection #980

Uh oh!

Conversation

q-inho commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

q-inho commented Jun 2, 2025

Uh oh!

burgholzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burgholzer Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

q-inho commented Jun 2, 2025

Uh oh!

burgholzer commented Jun 2, 2025

Uh oh!

q-inho commented Jun 2, 2025

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

q-inho commented Jun 4, 2025

Uh oh!

burgholzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

q-inho commented Jun 4, 2025

Uh oh!

burgholzer commented Jun 9, 2025

Uh oh!

q-inho commented Jun 9, 2025

Uh oh!

burgholzer commented Jun 9, 2025

Uh oh!

q-inho commented Jun 9, 2025

Uh oh!

burgholzer commented Jun 11, 2025

Uh oh!

burgholzer commented Jun 13, 2025

q-inho commented Jun 2, 2025 •

edited

Loading

burgholzer Jun 2, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading

burgholzer left a comment •

edited

Loading

q-inho commented Jun 13, 2025 •

edited

Loading