🚧 Mark-And-Sweep Garbage Collection #1020

MatthiasReumann · 2025-06-24T04:29:34Z

Description

Continues the work from #980 and resolves issue #644.

Checklist:

The pull request only contains commits that are focused and relevant to this change.
I have added appropriate tests that cover the new/changed functionality.
I have updated the documentation to reflect these changes.
I have added entries to the changelog for any noteworthy additions, changes, fixes or removals.
I have added migration instructions to the upgrade guide (if needed).
The changes follow the project's style guidelines and introduce no new warnings.
The changes are fully tested and pass the CI checks.
I have reviewed my own code changes.

…nagement in DD package

…r improved performance

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

…ounting and custom hash/equality functions for edges

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

…y Management

…r clarity

…age_DD_package

… in Windows

…truct

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

burgholzer

Just briefly had some time to look over this and wanted to leave some comments just in case. Many thanks for working on this 🙏

burgholzer · 2025-06-24T06:14:27Z

include/mqt-core/dd/Node.hpp

+  std::uint16_t flags = 0; // TODO: Would it make sense to use a larger datatype
+                           // here since Ref is gone?


Yeah. I was thinking about that already.
Initially, I would have hoped that we get some space savings for the nodes here, but that turned out to be wishful thinking. So we might as well make the padding explicit.
However that's also kind of awkward as there is no 48bit type.

What's your take on using a bit field? All I know is that they exist.

Hm. Just read through the linked page.
Sounds reasonable in principle. I am a bit worried that the standard text mentions the implementation defined nature of the allocation details quite often.
We should, at least, make sure that on the platforms that we commonly test with, the packing and alignment of the resulting struct is as we would expect it.

Yes. Quick googling tells us that this might not be the most portable and efficient solution.

So I guess we stick to 32 bit for now? 🤔

Yeah. Let's stick with that for now. We don't use most of the flag bitfield anyway.
So this is highly likely to be the most portable solution for now.

Thinking about BitFields made me wondering: Wouldn't it be nice to have something like:

struct Flags { // Size: 4 bytes, alignment 4 bytes uint32_t mark : 1; uint32_t reduced : 1; uint32_t dm : 1; uint32_t firstPath : 1; uint32_t conjugated : 1; }; Flags f;

The compiler would give us the respective masks for free. LLVM uses it in a similar fashion too, see here.

Anyhow, probably something for a follow-up PR.

That looks really reasonable. and convenient.
Seems to be something for a separate PR, but I definitely like the idea! 👍🏼

burgholzer · 2025-06-24T06:16:32Z

include/mqt-core/dd/Operations.hpp

@@ -101,8 +101,8 @@ MatrixDD getInverseDD(const qc::Operation& op, Package& dd,
 * @brief Apply a unitary operation to a given vector DD.
 *
 * @details This is a convenience function that realizes @p op times @p in and
- * correctly accounts for the permutation of the operation's qubits as well as
- * automatically handles reference counting.


I think I would personally prefer to keep the old wording in most of the places where this was changed. Essentially, we are still doing some kind of reference counting, but only at the top level.

burgholzer · 2025-06-24T06:23:29Z

include/mqt-core/dd/Package.hpp

+    template <class Edge> static void mark(const RootSet<Edge>& roots) {
+      for (auto& [edge, _] : roots) {
+        auto e = edge;
+        e.mark();
+      }
+    }
+
+    /// @brief Unmark edges contained in @p roots.
+    template <class Edge> static void unmark(const RootSet<Edge>& roots) {
+      for (auto& [edge, _] : roots) {
+        auto e = edge;
+        e.unmark();
+      }
+    }


Just so that I noted it down: I am still not quite sure if this trick of copying the hashmap key is actually valid.

I was wondering that too. Seems to work - but looks awkward. I think something like a const_cast could be useful here.

Maybe one could also just separately mark edge.p and edge.w. Maybe that would work without the copy because only the pointers are const, but not the data they point to 🤷🏼

Much easier solution: Make Edge::mark and Edge::unmark const functions 🫠

include/mqt-core/dd/UniqueTable.hpp

src/dd/Complex.cpp

src/dd/RealNumber.cpp

codecov · 2025-06-26T08:00:53Z

Codecov Report

Attention: Patch coverage is 99.09091% with 2 lines in your changes missing coverage. Please review.

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

📢 Thoughts on this report? Let us know!

test/dd/test_package.cpp

MatthiasReumann · 2025-06-26T09:56:02Z

👋🏻 @burgholzer

I think this is a good point to ask you for some input. The TODO:s in the code highlight the discussion points.

The current implementation holds a static .5 in the real unique table. Not used, this number will certainly be deleted - some tests have failed as a consequence of that. But: What's more interesting is that other tests - Approximation - fail due to numeric issues when removing this static value. For example, the TwoQubitCorrectlyRebuilt doesn't approximate anything with the exact budget of .25 but works for .26. Similarly, ThreeQubitRemoveNodeWithChildren.
For some reason the Grover/Grover.Functionality/15_qubits_2 is the only test that fails inconsistently on my system and those of the CI. I want to believe this is also due to numerical issues - but I'm not entirely sure either. In fact, this also happens for the recursive version of the test.
Using the track and untrack semantic requires a different philosophy for referencing counting in the project, I think. As of now it feels kind of awkward to use track and untrack throughout. Currently, it also seems a bit inconsistent in the way it is used.

Here's a suggestion: It is the callees job to track and untrack states. When returning states, the state should always be untracked. The function itself can change the tracked states but must always "clean up their own garbage". Essentially, this bowls then down to the end-user calling track and untrack for DD's they want to - well - track. I could even imagine that is possible to refactor VectorDD to a struct with constructor (track) and destructor (untrack) for something similar to the RAII idiom.

This is a major refactoring - for sure.

Thanks 🙇🏻

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

👋🏻 @burgholzer

I think this is a good point to ask you for some input. The TODO:s in the code highlight the discussion points.

The current implementation holds a static .5 in the real unique table. Not used, this number will certainly be deleted - some tests have failed as a consequence of that. But: What's more interesting is that other tests - Approximation - fail due to numeric issues when removing this static value. For example, the TwoQubitCorrectlyRebuilt doesn't approximate anything with the exact budget of .25 but works for .26. Similarly, ThreeQubitRemoveNodeWithChildren.

Yeah. This 0.5 turned out to be fairly important for numerical stability in experimental evaluations. Since 0.5 can be exactly represented as a FP number, it is highly beneficial to have it represented explicitly. A some point, we even thought about adding more numbers of the form 1/2^k to the table unconditionally.
That is actually one of the main reason for the RealNumberTable and all the managing we do in that regard instead of simply relying on complex numbers and straight-up multiplications.
One potential immediate solution here would be to add 0.5 to the statically defined numbers.
That adds another if check to all of the computations though. So I think I'd much rather have it baked into the respective unique table somehow.
Maybe one needs a way to mark a node/number as "immortal". Another flag?

For some reason the Grover/Grover.Functionality/15_qubits_2 is the only test that fails inconsistently on my system and those of the CI. I want to believe this is also due to numerical issues - but I'm not entirely sure either. In fact, this also happens for the recursive version of the test.

Yeah. That is highly likely to be numerical issues as well. Grover is particularly sensitive to these kinds of errors.
In the ideal case, the DD after every Grover iteration only consists of two strands: one uniform superposition strand of nodes with edge weights 1/sqrt(2) for every edge. And one that marks the solution strand for Grover.
The big problem is that numerical inaccuracies create a situation where the uniform superposition branch grows exponentially in terms of the numbers of nodes, while it is exponentially close to the ideal state in terms of fidelity.
That's a fairly fundamental problem that the current implementation of the DD package tries very hard to work around for certain scales of systems.

Using the track and untrack semantic requires a different philosophy for referencing counting in the project, I think. As of now it feels kind of awkward to use track and untrack throughout. Currently, it also seems a bit inconsistent in the way it is used.
Here's a suggestion: It is the callees job to track and untrack states. When returning states, the state should always be untracked. The function itself can change the tracked states but must always "clean up their own garbage". Essentially, this bowls then down to the end-user calling track and untrack for DD's they want to - well - track. I could even imagine that is possible to refactor VectorDD to a struct with constructor (track) and destructor (untrack) for something similar to the RAII idiom.
This is a major refactoring - for sure.

I would very much be open to have a more fundamental change to how references are being tracked as part of the package. I agree that it feels awkward at times. I am open to ideas for how to better do this and to automate it slightly better.
I'd just have one request: Can we extract some of the useful changes from here in a separate PR to reduce the size of the PR here?

I hope this helps.

…get of .25

MatthiasReumann · 2025-06-27T07:36:59Z

test/algorithms/test_grover.cpp

+    const auto next = dd->multiply(iterationOp, iteration);
+    dd->track(next);
+    dd->untrack(iteration); // This will automatically untrack the iterationOp.
+    iteration = next;


Interestingly, adding garbageCollect() here causes the numerical issues.

I suppose that might be because the tables are actually full enough so that collection happens. Without garbage collection, "dead" entries might become alive again in subsequent computations. With garbage collection, these entries might be gone and the computation might result in slightly different results due to tolerances and such.
The interesting thing is that I would not expect things to actually change based on the changes in this PR. We are still using the same criterion for when to collect garbage and we should be tracking the same DDs as previously with the more fine-grained reference counting.

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

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

a9d5ffe

…nagement in DD package

✨ 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

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

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

46e7aa9

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

Refactor RealNumber, ComplexNumber and UniqueTable for Improved Memor…

c88a319

…y Management

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

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]>

Merge branch 'mark_and_sweep_garbage_DD_package' into enh/mark-and-sweep

dbde227

Refactor existing solution

51d40d2

Use next pointer for pointer tagging

938a1f0

burgholzer reviewed Jun 24, 2025

View reviewed changes

MatthiasReumann added 5 commits June 24, 2025 11:44

Fix segfault when pointer tagging

a6a1af6

Remove unnecessary <iostream> include

7a4dc44

Properly track the states in dd functionality tests

7c54040

make dd's const

ca7c5cf

Add missing track statements

c5ecda7

MatthiasReumann added 3 commits June 26, 2025 06:34

Reorder untrack and track statements

7265260

Remove static 0.5 from cUniqueTable

3ede3c3

Restructure grover tests

95b6f13

burgholzer added enhancement New feature or request DD Anything related to the DD package c++ Anything related to C++ code labels Jun 26, 2025

MatthiasReumann added 2 commits June 26, 2025 09:57

Remove std::dec

c043a29

Use enable_if

797c7fd

github-advanced-security bot found potential problems Jun 26, 2025

View reviewed changes

test/dd/test_package.cpp Fixed Show fixed Hide fixed

test/dd/test_package.cpp Fixed Show fixed Hide fixed

MatthiasReumann added 4 commits June 26, 2025 11:04

Uncomment EXPECT_EQ statements

673aa98

Update python bindings

4dcfda7

Fix python test

7b57bf0

Add missing track statements

a63dcb4

Resolve linting issues

39d15a1

MatthiasReumann added 5 commits June 27, 2025 09:03

Add immortal numbers

f8963c4

Rename immortal flag

f0e34f1

Numerical issues resolved for Approximation Tests; revert back to bud…

6f0f429

…get of .25

Remove intermediate garbage collection

682d51d

Wrap RealNumber flags in anonymous namespace

6e04ee3

MatthiasReumann commented Jun 27, 2025

View reviewed changes

MatthiasReumann added 2 commits June 27, 2025 09:56

Make mark() and unmark() of Edge const

d858184

Add function descriptions

bbe4b64

		std::uint16_t flags = 0; // TODO: Would it make sense to use a larger datatype
		// here since Ref is gone?

🚧 Mark-And-Sweep Garbage Collection #1020

Are you sure you want to change the base?

🚧 Mark-And-Sweep Garbage Collection #1020

Uh oh!

Conversation

MatthiasReumann commented Jun 24, 2025

Description

Checklist:

Uh oh!

burgholzer left a comment

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!

MatthiasReumann Jun 27, 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!

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!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

burgholzer commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MatthiasReumann Jun 27, 2025 •

edited

Loading

codecov bot commented Jun 26, 2025 •

edited

Loading

MatthiasReumann commented Jun 26, 2025 •

edited

Loading