damccorm commented on code in PR #41162:
URL: https://github.com/apache/beam/pull/41162#discussion_r4528517076
##########
sdks/python/apache_beam/runners/common.py:
##########
@@ -1765,11 +1765,22 @@ def handle_process_batch_outputs(
then dispatched to the appropriate indexed output.
"""
if results is None:
- results = []
+ if self.per_element_output_counter is not None:
+ self.per_element_output_counter.add_input(0)
+ return
output_element_count = 0
+ empty_windowed_value = (
+ windowed_input_batch.as_empty_windowed_value()
+ if self._process_batch_yields_elements else None)
for result in results:
- tag, result = self._handle_tagged_output(result)
+ if isinstance(result, TaggedOutput):
+ tag = result.tag
+ if not isinstance(tag, str):
+ raise TypeError('In %s, tag %s is not a string' % (self, tag))
+ result = result.value
+ else:
+ tag = None
Review Comment:
Even though `_handle_tagged_output` is declared as `cdef inline tuple
_handle_tagged_output(self, result)` in `common.pxd`, its return type is
`tuple` (`PyObject*`). Because Cython does not eliminate tuple construction
across `cdef inline tuple` return boundaries, calling `tag, result =
self._handle_tagged_output(result)` compiles to `PyTuple_Pack(2, Py_None,
result)` (allocating a 2-tuple on every output) followed by unpacking and
`Py_DECREF` in the caller (and in pure Python, it also incurs a method lookup +
frame call + 2-tuple allocation per output).
Inlining `if isinstance(result, TaggedOutput): ... else: tag = None` avoids
the 2-tuple allocation/unpacking on every output. Happy to revert the inlining
here (and in #41161) if you prefer keeping `self._handle_tagged_output(result)`
for readability!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
