Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid copying unnecessary buffers between simulation iterations #4789

Merged
merged 26 commits into from
Jan 14, 2022
Merged

Avoid copying unnecessary buffers between simulation iterations #4789

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
993cf0c
Add a with_buffer parameter to ActOnArgs.copy
yjt98765 Dec 30, 2021
6abb2b9
Fix mypy error
yjt98765 Dec 31, 2021
9fa2bed
Merge branch 'master' into actonarg
yjt98765 Dec 31, 2021
a865b7e
Change copy's parameter to reuse_buffer
yjt98765 Jan 2, 2022
3af3ec4
Change the semantics of reuse_buffer parameter
yjt98765 Jan 3, 2022
2a556e4
Merge branch 'master' into actonarg
yjt98765 Jan 5, 2022
df67918
Add docstring and deprecation warning
yjt98765 Jan 5, 2022
2d53c76
Support default buffer parameters in ActOnArgs
yjt98765 Jan 5, 2022
4f10146
Fix CI errors
yjt98765 Jan 5, 2022
280ad3e
Fix test_state_vector_trial_result_repr
yjt98765 Jan 5, 2022
5f821e4
Add test for deprecation warnings
yjt98765 Jan 5, 2022
5edf97f
Fix CI errors
yjt98765 Jan 5, 2022
7bc4908
Merge branch 'master' into actonarg
yjt98765 Jan 6, 2022
412c1bc
Use assert_deprecated for deprecation test
yjt98765 Jan 6, 2022
0480241
Add a test case for the deprecation warning in _run
yjt98765 Jan 6, 2022
b2fda13
Fix coverage and type errors
yjt98765 Jan 6, 2022
a006a39
Fix a coverage error
yjt98765 Jan 6, 2022
49cb93d
Merge branch 'master' into actonarg
yjt98765 Jan 7, 2022
67141cb
Merge branch 'master' into actonarg
yjt98765 Jan 12, 2022
8e8076b
Raise a ValueError when qid_shape cannot be inferred
yjt98765 Jan 12, 2022
3369294
Fix type hint and deprecation deadline problems
yjt98765 Jan 13, 2022
7f7ff17
Rename reuse_buffer to deep_copy_buffers
yjt98765 Jan 13, 2022
195b802
Merge branch 'master' into actonarg
yjt98765 Jan 14, 2022
62addd1
Add shallow copy logic to copy method
yjt98765 Jan 14, 2022
2b948a9
Merge branch 'master' into actonarg
yjt98765 Jan 14, 2022
5d260b1
Merge branch 'master' into actonarg
CirqBot Jan 14, 2022
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
2 changes: 1 addition & 1 deletion cirq-core/cirq/contrib/quimb/mps_simulator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ def __str__(self) -> str:
def _value_equality_values_(self) -> Any:
return self.qubit_map, self.M, self.simulation_options, self.grouping

def _on_copy(self, target: 'MPSState'):
def _on_copy(self, target: 'MPSState', reuse_buffer: bool = False):
target.simulation_options = self.simulation_options
target.grouping = self.grouping
target.M = [x.copy() for x in self.M]
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/protocols/act_on_protocol_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def __init__(self, fallback_result: Any = NotImplemented, measurements=None):
def _perform_measurement(self, qubits):
return self.measurements # coverage: ignore

def copy(self):
def copy(self, reuse_buffer: bool = False):
return DummyActOnArgs(self.fallback_result, self.measurements.copy()) # coverage: ignore

def _act_on_fallback_(
Expand Down
6 changes: 3 additions & 3 deletions cirq-core/cirq/sim/act_on_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,14 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
"""Child classes that perform measurements should implement this with
the implementation."""

def copy(self: TSelf) -> TSelf:
def copy(self: TSelf, reuse_buffer: bool = False) -> TSelf:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's definitely a breaking change here, in that any external subclass of ActOnArgs (if one exists...) with its own definition of copy will break when copy is called on it by Cirq code that uses both arguments.

In hindsight, the right way to prevent this probably involves private classes (which Python is allergic to), but given how heavily internal this code is I think we can get away with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We managed to make this non breaking with some reflection. Hence the test that still works. Warning messages are appropriately emitted when the new argument does not exist (and there are tests around that as well).

This could be a good reference for doing such things.

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, if we check the signature before actually calling the copy (like the case in SimulatorBase._run), we can avoid errors.

"""Creates a copy of the object."""
args = copy.copy(self)
self._on_copy(args)
self._on_copy(args, reuse_buffer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure to do a if 'reuse_buffer' in inspect.signature(_on_copy) check here for backwards compatibility, and output a deprecation warning in the else branch.

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 have added the deprecation warning and a test for it, but I am not sure whether I am doing it in the right way. I can continue improving it based on your feedback.

args._log_of_measurement_results = self.log_of_measurement_results.copy()
return args

def _on_copy(self: TSelf, args: TSelf):
def _on_copy(self: TSelf, args: TSelf, reuse_buffer: bool = False):
"""Subclasses should implement this with any additional state copy
functionality."""

Expand Down
4 changes: 2 additions & 2 deletions cirq-core/cirq/sim/act_on_args_container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ def _act_on_fallback_(
self.args[q] = op_args
return True

def copy(self) -> 'cirq.ActOnArgsContainer[TActOnArgs]':
def copy(self, reuse_buffer: bool = False) -> 'cirq.ActOnArgsContainer[TActOnArgs]':
logs = self.log_of_measurement_results.copy()
copies = {a: a.copy() for a in set(self.args.values())}
copies = {a: a.copy(reuse_buffer) for a in set(self.args.values())}
for copy in copies.values():
copy._log_of_measurement_results = logs
args = {q: copies[a] for q, a in self.args.items()}
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/sim/act_on_args_container_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def __init__(self, qubits, logs):
def _perform_measurement(self, qubits: Sequence[cirq.Qid]) -> List[int]:
return [0] * len(qubits)

def copy(self) -> 'EmptyActOnArgs':
def copy(self, reuse_buffer: bool = False) -> 'EmptyActOnArgs':
return EmptyActOnArgs(
qubits=self.qubits,
logs=self.log_of_measurement_results.copy(),
Expand Down
5 changes: 3 additions & 2 deletions cirq-core/cirq/sim/act_on_density_matrix_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,10 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
)
return bits

def _on_copy(self, target: 'cirq.ActOnDensityMatrixArgs'):
def _on_copy(self, target: 'cirq.ActOnDensityMatrixArgs', reuse_buffer: bool = False):
target.target_tensor = self.target_tensor.copy()
target.available_buffer = [b.copy() for b in self.available_buffer]
if reuse_buffer:
target.available_buffer = [b.copy() for b in self.available_buffer]

def _on_kronecker_product(
self, other: 'cirq.ActOnDensityMatrixArgs', target: 'cirq.ActOnDensityMatrixArgs'
Expand Down
5 changes: 3 additions & 2 deletions cirq-core/cirq/sim/act_on_state_vector_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
)
return bits

def _on_copy(self, target: 'cirq.ActOnStateVectorArgs'):
def _on_copy(self, target: 'cirq.ActOnStateVectorArgs', reuse_buffer: bool = False):
target.target_tensor = self.target_tensor.copy()
target.available_buffer = self.available_buffer.copy()
if reuse_buffer:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if not reuse_buffer here and above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is: if the buffer is copied to the new object, then it can be "reused"; otherwise, the object might need to create its own buffer (from scratch). The point is, the context of the parameter is only the copy method. Its semantics should not be attached to a specific usage such as _run. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In act_on_args.copy in line 118, it does a shallow copy, meaning that if we don't do anything, then the buffer instance itself is not copied. Thus if reuse_buffer is set here, that means we don't have to do anything else. If reuse_buffer is false, then that means the calling function wants to forcibly deep-copy the buffer. So this condition needs changed to if not reuse_buffer.

I'm not quite sure I catch the question about semantics, but whether-or-not-to-copy-the-buffer is context-dependent, not data-structure-dependent. If the calling function is running repetitions serially (as _run does), then the buffer can be safely reused from one repetition to another. If the user is running them in parallel, then all referencing the same buffer is obviously unsafe. So it depends on the use case.

target.available_buffer = self.available_buffer.copy()

def _on_kronecker_product(
self, other: 'cirq.ActOnStateVectorArgs', target: 'cirq.ActOnStateVectorArgs'
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/sim/clifford/act_on_clifford_tableau_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
"""Returns the measurement from the tableau."""
return [self.tableau._measure(self.qubit_map[q], self.prng) for q in qubits]

def _on_copy(self, target: 'ActOnCliffordTableauArgs'):
def _on_copy(self, target: 'ActOnCliffordTableauArgs', reuse_buffer: bool = False):
target.tableau = self.tableau.copy()

def sample(
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/sim/clifford/act_on_stabilizer_ch_form_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
"""Returns the measurement from the stabilizer state form."""
return [self.state._measure(self.qubit_map[q], self.prng) for q in qubits]

def _on_copy(self, target: 'ActOnStabilizerCHFormArgs'):
def _on_copy(self, target: 'ActOnStabilizerCHFormArgs', reuse_buffer: bool = False):
target.state = self.state.copy()

def sample(
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/sim/operation_target.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def apply_operation(self, op: 'cirq.Operation'):
protocols.act_on(op, self)

@abc.abstractmethod
def copy(self: TSelfTarget) -> TSelfTarget:
def copy(self: TSelfTarget, reuse_buffer: bool = False) -> TSelfTarget:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure to add Args to the docstring with an explanation of reuse_buffer. (Not just "True to reuse the buffer". Something like "If True, any buffers will be reused by the copy. This will save time by avoiding a deep copy of the buffers, but should only be used when there is no chance that the two objects will be writing to the buffers simultaneously.")

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 have added the paragraph to the docstring, but I do not fully understand it. It seems that there are no buffers shared between the iterations of the simulation.

Cirq/cirq-core/cirq/sim/simulator_base.py

Lines 268 to 278 in b60b9f8

for i in range(repetitions):
all_step_results = self._core_iterator(
general_suffix,
sim_state=act_on_args.copy() if i < repetitions - 1 else act_on_args,
)
for step_result in all_step_results:
pass
for k, v in step_result.measurements.items():
if k not in measurements:
measurements[k] = []
measurements[k].append(np.array(v, dtype=np.uint8))

I cannot find where the shared buffer is stored. It is not in ActOnDensityMatrixArgs because not copying the buffer does not affect the simulation. And it is not in DensityMatrixSimulator either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment old? You have updated the function in simulator_base to apply the resuse_buffers=True option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have not discussed it yet and I still hold the same opinion. I agree that without copying buffers would improve performance. I am just not sure about the name reuse_buffer and the explanation in the docstring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, how would you phrase it? My wording probably wasn't the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we say skip_buffer? The docstring can be something like True to skip copying buffers. I think we can just describe the behavior related to the parameter instead of the purpose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that sounds to me like it would just set the new object buffer to null. But I think I understand your point in that reuse_buffers could be confusing too. Maybe something more direct like shallow_copy_buffers?

Copy link
Contributor Author

@yjt98765 yjt98765 Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I support a more direct way. But, is the buffer shallow copied? I only write

        if not reuse_buffer:
            target.available_buffer = [b.copy() for b in self.available_buffer]

in act_on_density_matrix_args.py. Is this what you expected? I did not assign anything to target.available_buffer if reuse_buffer is True, because I think it will simply not be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well by "shallow copied", I mean the reference to available_buffer is copied, which occurs in ActOnArgs.copy. So documentation-wise, I guess it depends on whether you're viewing it from the perspective of the user who is calling copy or the perspective of the user who is implementing a new subclass of ActOnArgs.

Maybe clearer in both situations would be to call it deep_copy_buffers, and default that to True. I always prefer naming booleans such that the default is False, but perhaps it's more clear to use deep_copy_buffers=True as default here. (There are plenty of other places where we default a boolean to True, so it wouldn't be an oddball or anything). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I will update the code soon.

"""Copies the object."""

@property
Expand Down
2 changes: 1 addition & 1 deletion cirq-core/cirq/sim/simulator_base_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def _perform_measurement(self, qubits: Sequence['cirq.Qid']) -> List[int]:
self.measurement_count += 1
return [self.gate_count]

def copy(self) -> 'CountingActOnArgs':
def copy(self, with_buffer: bool = True) -> 'CountingActOnArgs':
args = CountingActOnArgs(
qubits=self.qubits,
logs=self.log_of_measurement_results.copy(),
Expand Down