EncourageReuse option in QubitManager. #320
Conversation
Currently Qubit Manager tries to reuse released qubits as soon as possible and prefers it to allocating fresh unused ones. This is good for some scenarios, however, in other scenarios it may be preferrable to first allocate fresh unused qubits, and only then try to reuse released ones (oldest released first). Both behaviors are now possible via a backwards-compatible optional parameter in QubitManager constructor.
| /// Test for QubitManager. | ||
| /// </summary> | ||
| [Fact] | ||
| public void TestQubitManagerDiscouragingReuse() |
There was a problem hiding this comment.
Split into two tests so it will be easier to localize test failures? Also, it would be nice to have the same allocation pattern tested against a qubit manager configure with encourageReuse: true.
There was a problem hiding this comment.
Yes, the same allocation pattern is being tested, just in another function. I am trying to avoid having too many test functions. We already have about 300 and navigating that list is a concern. So I am trying to keep small related tests together. This way it is also easier to understand they are doing the same. See for yourself, you did notice the same pattern in two blocks because they were in the same function, but you didn't notice the same pattern in a function right above it.
There was a problem hiding this comment.
I didn't look at the function above, because that code didn't change -- that's an inherent danger of code reviews. If I was working with code (to change it, etc.) I'd look through test cases' names and this is where having well named separate tests helps to build the mental picture of what is covered and what matters. The other benefit of smaller tests is that when a test fails you get a localized failure, identified by the test's name. Might not be a big deal on a local machine when you can rerun tests under debugger, but important for CIs.
| Debug.Assert(this.qubits[NumQubits - 1] == None); | ||
|
|
||
| free = 0; | ||
| freeTail = NumQubits - 1; |
There was a problem hiding this comment.
Given it's only valid for EncourageReuse=false, would it make sense to set it to -1 (or some other 'invalid' sentinel value) when EncourageReuse=true?
There was a problem hiding this comment.
I don't see why. It is only used in discourage reuse cases, but it is valid always. It is OK either way but it will take more churn to maintain -1 value. Every time you set it you have to do an if? Not worth it.
There was a problem hiding this comment.
The comment for freeTail was saying that it's only valid when EncourageReuse=false. When a state of a class is allowed to be inconsistent, I like making it more explicit with "invalid" markers so can assert on them and spot problems under debugger easier. In this case, probably should just change to comment to say "only used when EncourageReuse=false".
| long AllocatedForBorrowing; // All qubits allocated only for borrowing, will be marked with this number or higher. | ||
| long[] qubits; // Tracks the allocation state of all qubits. | ||
| long free; // Points to the first free (unallocated) qubit. | ||
| long freeTail; // Points to the last free (unallocated) qubit. Only valid iff (!EncourageReuse). |
There was a problem hiding this comment.
For my education: why do we need dynamic Allocated/Disabled markers rather than using fixed sentinel values (to avoid remarking the qubits when the manager is extended)?
There was a problem hiding this comment.
I'd also suggest renaming free to freeHead to match freeTail (and maybe expanding the comment, explaining that this is a singly-linked free list).
Currently Qubit Manager tries to reuse released qubits as soon as possible and prefers it to allocating fresh unused ones. This is good for some scenarios, however, in other scenarios it may be preferrable to first allocate fresh unused qubits, and only then try to reuse released ones (oldest released first). Both behaviors are now possible via a backwards-compatible optional parameter in QubitManager constructor.