Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

EncourageReuse option in QubitManager. #320

Merged
merged 3 commits into from
Aug 4, 2020
Merged

EncourageReuse option in QubitManager. #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdfee09
EncourageReuse option in QubitManager.
avasch01 Aug 3, 2020
eb0a92d
Review feedback
avasch01 Aug 4, 2020
6d01b32
.
avasch01 Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions src/Simulation/Common/QubitManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Microsoft.Quantum.Intrinsic;
using Microsoft.Quantum.Simulation.Core;
using Microsoft.Quantum.Simulation.Simulators.Exceptions;

Expand All @@ -25,11 +26,13 @@ public class QubitManager
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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

long numAllocatedQubits; // Tracking this for optimization.
long numDisabledQubits; // Number of disabled qubits.

// Options
bool MayExtendCapacity;
bool EncourageReuse;
public bool DisableBorrowing { get; }

const long MaxQubitCapacity = long.MaxValue - 3;
Expand All @@ -49,9 +52,14 @@ public class QubitManager
/// <summary>
/// Creates and initializes QubitManager that can handle up to numQubits qubits
/// </summary>
public QubitManager(long qubitCapacity, bool mayExtendCapacity = false, bool disableBorrowing = false)
public QubitManager(
long qubitCapacity,
bool mayExtendCapacity = false,
bool disableBorrowing = false,
bool encourageReuse = true)
{
MayExtendCapacity = mayExtendCapacity;
EncourageReuse = encourageReuse;
DisableBorrowing = disableBorrowing;

if (qubitCapacity <= 0) { qubitCapacity = MinQubitCapacity; }
Expand All @@ -65,6 +73,7 @@ public QubitManager(long qubitCapacity, bool mayExtendCapacity = false, bool dis
Debug.Assert(this.qubits[NumQubits - 1] == None);

free = 0;
freeTail = NumQubits - 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

numAllocatedQubits = 0;
numDisabledQubits = 0;
}
Expand Down Expand Up @@ -121,6 +130,7 @@ private void ExtendQubitArray()
if (free == oldNone)
{
free = oldNumQubits;
freeTail = NumQubits - 1;
} else
{
Debug.Assert(false, "Why do we extend an array, when we still have available slots?");
Expand Down Expand Up @@ -300,8 +310,29 @@ protected virtual void ReleaseOneQubit(Qubit qubit, bool usedOnlyForBorrowing)
{
throw new ArgumentException("Attempt to free qubit that has not been allocated.");
}
qubits[qubit.Id] = free;
free = qubit.Id;
if (EncourageReuse) {
qubits[qubit.Id] = free;
free = qubit.Id;
}
else
{
// If we are allowed to extend capacity we will never reuse this qubit,
// otherwise we need to add it to the free qubits list.
if (!MayExtendCapacity)
{
if (qubits[freeTail] != None)
{
// There were no free qubits at all
free = qubit.Id;
}
else
{
qubits[freeTail] = qubit.Id;
}
}
qubits[qubit.Id] = None;
freeTail = qubit.Id;
}

numAllocatedQubits--;
Debug.Assert(numAllocatedQubits >= 0);
Expand Down
8 changes: 6 additions & 2 deletions src/Simulation/Common/QubitManagerTrackingScope.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,12 @@ public List<Qubit> Locals
private Stack<StackFrame> operationStack; // Stack of operation calls.
private StackFrame curFrame; // Current stack frame - all qubits in current scope are listed here.

public QubitManagerTrackingScope(long qubitCapacity, bool mayExtendCapacity = false, bool disableBorrowing = false)
: base(qubitCapacity, mayExtendCapacity, disableBorrowing)
public QubitManagerTrackingScope(
long qubitCapacity,
bool mayExtendCapacity = false,
bool disableBorrowing = false,
bool encourageReuse = true)
: base(qubitCapacity, mayExtendCapacity, disableBorrowing, encourageReuse)
{
if (!DisableBorrowing)
{
Expand Down
85 changes: 85 additions & 0 deletions src/Simulation/Simulators.Tests/QubitManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,91 @@ public void TestQubitManager()
}
}

/// <summary>
/// Test for QubitManager.
/// </summary>
[Fact]
public void TestQubitManagerDiscouragingReuse()
Copy link
Contributor

@IrinaYatsenko IrinaYatsenko Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
{ // BLOCK testing mayExtendCapacity:false
QubitManager qm = new QubitManager(10, mayExtendCapacity: false, disableBorrowing: false, encourageReuse: false);

// Test allocation of single qubit
Qubit q1 = qm.Allocate();
Assert.True(q1.Id == 0);

// Test allocation of multiple qubits
IQArray<Qubit> qa1 = qm.Allocate(4);
Assert.True(qa1.Length == 4);
Assert.True(qa1[0].Id == 1);
Assert.True(qa1[1].Id == 2);
Assert.True(qa1[2].Id == 3);
Assert.True(qa1[3].Id == 4);

// Test reuse of deallocated qubits
qm.Release(qa1[1]);

Qubit q2 = qm.Allocate();
Assert.True(q2.Id == 5);

IQArray<Qubit> qa2 = qm.Allocate(3);
Assert.True(qa2.Length == 3);
Assert.True(qa2[0].Id == 6);
Assert.True(qa2[1].Id == 7);
Assert.True(qa2[2].Id == 8);

qm.Release(qa2);

Qubit q3 = qm.Allocate();
Assert.True(q3.Id == 9);

Qubit q4 = qm.Allocate();
Assert.True(q4.Id == 2);

Qubit q5 = qm.Allocate();
Assert.True(q5.Id == 8);
}

{ // BLOCK testing mayExtendCapacity:true
QubitManager qm = new QubitManager(10, mayExtendCapacity: true, disableBorrowing: false, encourageReuse: false);

// Test allocation of single qubit
Qubit q1 = qm.Allocate();
Assert.True(q1.Id == 0);

// Test allocation of multiple qubits
IQArray<Qubit> qa1 = qm.Allocate(4);
Assert.True(qa1.Length == 4);
Assert.True(qa1[0].Id == 1);
Assert.True(qa1[1].Id == 2);
Assert.True(qa1[2].Id == 3);
Assert.True(qa1[3].Id == 4);

// Test reuse of deallocated qubits
qm.Release(qa1[1]);

Qubit q2 = qm.Allocate();
Assert.True(q2.Id == 5);

IQArray<Qubit> qa2 = qm.Allocate(3);
Assert.True(qa2.Length == 3);
Assert.True(qa2[0].Id == 6);
Assert.True(qa2[1].Id == 7);
Assert.True(qa2[2].Id == 8);

qm.Release(qa2);

Qubit q3 = qm.Allocate();
Assert.True(q3.Id == 9);

Qubit q4 = qm.Allocate();
Assert.True(q4.Id == 10);

Qubit q5 = qm.Allocate();
Assert.True(q5.Id == 11);
}
}

/// <summary>
/// Test for QubitManagerTrackingScope.
/// </summary>
Expand Down