Repository navigation
Conversation
|
/gemini review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR introduces a Cython-friendly fast path in AbstractComponentCoderImpl for 2-component (KV) tuples by caching _coder_0, _coder_1, _num_coders, and _is_kv at initialization time. The optimization is clean, preserves existing wire format and nesting semantics, and yields substantial throughput gains for KV encoding/decoding.
🔍 General Feedback
- In
decode_from_stream, the non-KV fallback path replaced thec-bound loop variable withself._coder_impls[i].decode_from_stream(...), which loses theCoderImplstatic Cython type ontupleindexing and falls back to Python attribute lookup. coder_impl.pxddeclareslast=intin@cython.localsforget_estimated_size_and_observables, wherelastis not defined.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| c.decode_from_stream( | ||
| in_stream, nested or i + 1 < len(self._coder_impls)) | ||
| for i, c in enumerate(self._coder_impls) | ||
| self._coder_impls[i].decode_from_stream(in_stream, nested or i < last) |
There was a problem hiding this comment.
🟡 Medium: Because _coder_impls is typed as cdef tuple, indexing self._coder_impls[i] returns an untyped Python object rather than CoderImpl. Previously, for i, c in enumerate(self._coder_impls) bound c to @cython.locals(c=CoderImpl) so c.decode_from_stream(...) used Cython's C-level cpdef vtable dispatch. Casting with cython.cast(CoderImpl, self._coder_impls[i]) preserves the typed C call for non-2-component tuples.
| self._coder_impls[i].decode_from_stream(in_stream, nested or i < last) | |
| cython.cast(CoderImpl, self._coder_impls[i]).decode_from_stream(in_stream, nested or i < last) |
There was a problem hiding this comment.
Fixed in 7e134ed: kept for i, c in enumerate(self._coder_impls) so c remains typed as CoderImpl via @cython.locals(c=CoderImpl, i=int, last=int).
| cpdef decode_from_stream(self, InputStream stream, bint nested) | ||
|
|
||
| @cython.locals(c=CoderImpl) | ||
| @cython.locals(c=CoderImpl, i=int, last=int) |
There was a problem hiding this comment.
🟢 Low: get_estimated_size_and_observables in coder_impl.py was not updated to define last, so last=int in @cython.locals here is unused (and c=CoderImpl on line 163 is also unused if decode_from_stream does not bind a local variable c).
| @cython.locals(c=CoderImpl, i=int, last=int) | |
| @cython.locals(c=CoderImpl, i=int) |
There was a problem hiding this comment.
Fixed in 7e134ed: reverted @cython.locals on get_estimated_size_and_observables back to @cython.locals(c=CoderImpl).
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR adds a fast path for 2-component (KV) coders in AbstractComponentCoderImpl.encode_to_stream and AbstractComponentCoderImpl.decode_from_stream, along with corresponding Cython attribute and local variable declarations in coder_impl.pxd. The implementation preserves exact nested encoding/decoding semantics and component length validation while eliminating loop and list-allocation overhead on the hot path.
🔍 General Feedback
- The 2-component unrolling accurately preserves the
nested=Truebehavior for the first component and forwardsnestedto the second component. - Passing a 2-tuple directly to
_construct_from_componentsindecode_from_streamcleanly takes advantage ofTupleCoderImpl._construct_from_components(tuple(components)) returning the existing tuple without extra allocation. - Cython
cdefattributes and@cython.localsdeclarations incoder_impl.pxdare properly aligned with the implementation incoder_impl.py.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41216 +/- ##
============================================
+ Coverage 57.48% 57.64% +0.15%
Complexity 2492 2492
============================================
Files 1204 1198 -6
Lines 171265 171215 -50
Branches 2103 2103
============================================
+ Hits 98456 98700 +244
+ Misses 70157 69863 -294
Partials 2652 2652
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
damccorm
added
the
ready-for-human
Description
Optimizes
AbstractComponentCoderImplinsdks/python/apache_beam/coders/coder_impl.pyandsdks/python/apache_beam/coders/coder_impl.pxd:KV) fast path inencode_to_streamanddecode_from_stream:_num_coders,_is_kv,_coder_0, and_coder_1whenlen(self._coder_impls) == 2so 2-tuples (KVpairs) encode and decode directly withoutrange()loop overhead or list comprehension +tuple()allocation on every element.Benchmarks & Performance Impact
upstream/mastervsupstream/master+ this PR alone):bench_17_component_coder_kv_fastpath(c2feb961eef) (on branchperf/py-benchmarks-suite) — 500,000 2-tuple (KV) encode + decode round-trips inTupleCoderImplimproved from220.40 ms->124.94 ms(1.76xfaster,-43.3%,-95.46 ms) in compiled Cython.60.7%of stream savings,-38.66 ms): 2-componentencode_to_streamfast path (_coder_0.encode_to_stream/_coder_1.encode_to_stream) —74.72 ms->36.06 ms(2.07xfaster,-51.7%).39.3%of stream savings,-25.06 ms): 2-componentdecode_from_streamfast path ((_coder_0.decode_from_stream, _coder_1.decode_from_stream)without list comprehension +tuple()) —62.73 ms->37.67 ms(1.67xfaster,-39.9%).core_window_row_combine): Included inrun_core_window_row_combine_pipeline(c2feb961eef) (3,000,000 generated events -> 9,000,000 sliding-windowed elements on a singlen2-standard-4worker), which improved end-to-end worker stage execution time by 1.80x (-44.5%,599.6s->332.9s) and reduced total billable Dataflow vCPU/Memory/PD usage by-26.1%(1,134->838vCPU-s).Testing
pytest sdks/python/apache_beam/coders/coders_test.py sdks/python/apache_beam/coders/fast_coders_test.py.