Details
-
Bug
-
Status: Resolved
-
P3
-
Resolution: Fixed
-
None
-
None
Description
Beam Python 3 conversion exposed non-trivial performance-sensitive logic in element-batching transform. Let's take a look at util.py#L271.
Due to Python 2 language semantics, the result of x2 / x1 will depend on the type of the keys - whether they are integers or floats.
The keys of key-value pairs contained in self._data are added as integers here, however, when we 'thin' the collected entries here, the keys will become floats. Surprisingly, using either integer or float division consistently in the comparator negatively affects the performance of a custom pipeline I was using to benchmark these changes. The performance impact likely comes from changes in the logic that depends on how division is evaluated, not from the performance of division operation itself.
In terms of Python 3 conversion the best course of action that avoids regression seems to be to preserve the existing Python 2 behavior using old_div from past.utils.division, in the medium term we should clean up the logic. We may want to add a targeted microbenchmark to evaluate performance of this code, and maybe cythonize the code, since it seems to be performance-sensitive.
