Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion sdks/python/apache_beam/coders/coder_impl.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ cdef class TimestampCoderImpl(StreamCoderImpl):

cdef list small_ints
cdef class VarIntCoderImpl(StreamCoderImpl):
@cython.locals(ivalue=libc.stdint.int64_t)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a common (hot) code path. There might be performance implication here as it added multiple branches and removed this libc.stdint.int64_t C type.

Did some local benchmark

current HEAD:

VarIntCoderImpl benchmark (SLOW_STREAM=False)
Iterations per test: 20,000,000

--- Small Integers (<32767) ---
Encode: 2.4943s (8,018,329 ops/sec)
Decode: 1.1410s (17,528,902 ops/sec)

--- Large Integers (64-bit) ---
Encode: 3.1621s (6,324,850 ops/sec)
Decode: 1.3801s (14,492,127 ops/sec)

reverting this change:

VarIntCoderImpl benchmark (SLOW_STREAM=False)
Iterations per test: 20,000,000

--- Small Integers (<32767) ---
Encode: 1.9062s (10,492,259 ops/sec)
Decode: 1.1310s (17,683,731 ops/sec)

--- Large Integers (64-bit) ---
Encode: 2.5946s (7,708,456 ops/sec)
Decode: 1.4151s (14,133,459 ops/sec)

Ran it several times and the difference appears to be significant

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We even had dedicated micro-optimization for its Java counterpart: #29689

cpdef bytes encode(self, value)


Expand Down
34 changes: 26 additions & 8 deletions sdks/python/apache_beam/coders/coder_impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@
is_compiled = False
fits_in_64_bits = lambda x: -(1 << 63) <= x <= (1 << 63) - 1


def _as_signed_int64(value):
# type: (int) -> int

"""Folds a uint64 to the signed int64 with the same bits (and VarInt
encoding), which the Cython int64_t stream params accept. Values outside the
64-bit range raise here so the pure-Python path matches Cython's overflow."""
if (1 << 63) <= value < (1 << 64):
return int(value) - (1 << 64)
if not fits_in_64_bits(value):
raise OverflowError("%d is out of range for a 64-bit integer." % value)
return value
Comment thread
AviKndr marked this conversation as resolved.


if TYPE_CHECKING or SLOW_STREAM:
from .slow_stream import ByteCountingOutputStream
from .slow_stream import InputStream as create_InputStream
Expand Down Expand Up @@ -1044,22 +1058,26 @@ class VarIntCoderImpl(StreamCoderImpl):
def encode_to_stream(self, value, out, nested):
# type: (int, create_OutputStream, bool) -> None
try:
out.write_var_int64(value)
# Fold uint64 values into signed int64 so Cython doesn't overflow.
out.write_var_int64(_as_signed_int64(value))
except OverflowError as e:
raise OverflowError(
f"Integer value '{value}' is out of the encodable range for "
f"VarIntCoder. This coder is limited to values that fit "
f"within a 64-bit signed integer (-(2**63) to 2**63 - 1). "
f"VarIntCoder. This coder is limited to 64-bit integers: the "
f"signed range -(2**63) to 2**63 - 1, plus unsigned values up to "
f"2**64 - 1 which share the same wire encoding. "
f"Original error: {e}") from e

def decode_from_stream(self, in_stream, nested):
# type: (create_InputStream, bool) -> int
return in_stream.read_var_int64()

def encode(self, value):
ivalue = value # type cast
if 0 <= ivalue < len(small_ints):
return small_ints[ivalue]
# Compare as a Python object: a uint64 value overflows the int64_t cast
# the compiled fast path used to do here. Non-small values (including
# uint64) fall through to encode_to_stream, which folds them.
if 0 <= value < len(small_ints):
return small_ints[value]
return StreamCoderImpl.encode(self, value)

def decode(self, encoded):
Expand All @@ -1073,11 +1091,11 @@ def estimate_size(self, value, nested=False):
# type: (Any, bool) -> int
# Note that VarInts are encoded the same way regardless of nesting.
try:
return get_varint_size(value)
return get_varint_size(_as_signed_int64(value))
except OverflowError as e:
raise OverflowError(
f"Cannot estimate size for integer value '{value}'. "
f"Value is out of the range for VarIntCoder (64-bit signed integer). "
f"Value is out of the range for VarIntCoder (64-bit integer). "
f"Original error: {e}") from e


Expand Down
18 changes: 18 additions & 0 deletions sdks/python/apache_beam/coders/coders_test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,24 @@ def test_varint_coder(self):
for k in range(0, int(math.log(MAX_64_BIT_INT)))
])

def test_varint_coder_uint64(self):
# uint64 values [2**63, 2**64) must encode like the signed int64 with the
# same bits instead of overflowing Cython's int64_t. Decoding is signed,
# matching Java's VarIntCoder.
coder = coders.VarIntCoder()
impl = coder.get_impl()
for v in [1 << 63, (1 << 63) + 12345, (1 << 64) - 1]:
signed_twin = v - (1 << 64)
encoded = coder.encode(v)
self.assertEqual(encoded, coder.encode(signed_twin))
self.assertEqual(impl.estimate_size(v), len(encoded))
self.assertEqual(coder.decode(encoded), signed_twin)

# Values outside the 64-bit range stay out of range on both paths.
for v in [1 << 64, (1 << 70), -(1 << 63) - 1]:
with self.assertRaises(OverflowError):
coder.encode(v)

def test_varint32_coder(self):
# Small ints.
self.check_coder(coders.VarInt32Coder(), *range(-10, 10))
Expand Down
Loading