Abacn commented on code in PR #39047: URL: https://github.com/apache/beam/pull/39047#discussion_r3509521379
########## sdks/python/apache_beam/coders/coder_impl.pxd: ########## @@ -135,7 +135,6 @@ cdef class TimestampCoderImpl(StreamCoderImpl): cdef list small_ints cdef class VarIntCoderImpl(StreamCoderImpl): - @cython.locals(ivalue=libc.stdint.int64_t) Review Comment: 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 -- 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]
