Repository navigation
Conversation
- Inline `_next_uint()` in Cython `LCGenerator` (`fast_test_utils.pyx`) to eliminate two `cpdef` calls (`next_uint` -> `seed`) per 4-byte chunk in `randbytes`. - Generate key and value bytes separately in `SyntheticSource._gen_kv_pair` when `gen_algo == 'lcg'` and `key_size % 4 == 0` to avoid allocating and slicing an intermediate combined `bytes` object while preserving identical output for any seed.
There was a problem hiding this comment.
📋 Review Summary
This PR optimizes Cython LCGenerator and SyntheticSource._gen_kv_pair by introducing an inlined cdef helper _next_uint (avoiding cpdef overhead in tight loops) and generating key/value bytes directly when gen_algo == 'lcg' and key_size is 4-byte aligned. The changes are mathematically sound, preserve bit-for-bit identical output for any seed, and deliver substantial performance improvements.
🔍 General Feedback
- Inlining
_next_uintwithnoexcept nogilinfast_test_utils.pyxcleanly eliminates the repeatedcpdefcall overhead inrandbytes,next_int,next_uint, andrandom_sample. - The
_gen_kv_separatelycondition (self.gen_algo == 'lcg' and self._key_size % 4 == 0) accurately ensures that splitting therandbytescall does not alter the PRNG state progression compared to slicing a single combined buffer. - Consider adding a unit test in
synthetic_pipeline_test.pycoveringalgorithm='lcg'with both 4-byte aligned and unalignedkeySizeBytesto explicitly guard the new_gen_kv_separatelybranch.
💡 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 #40915 +/- ##
==========================================
+ Coverage 79.83% 80.08% +0.24%
==========================================
Files 494 494
Lines 83575 83585 +10
==========================================
+ Hits 66722 66935 +213
+ Misses 16853 16650 -203
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:
|
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Inline
_next_uint()in CythonLCGenerator(fast_test_utils.pyx) to eliminate twocpdefcalls (next_uint->seed) per 4-byte chunk inrandbytes.Generate key and value bytes separately in
SyntheticSource._gen_kv_pairwhengen_algo == 'lcg'andkey_size % 4 == 0to avoid allocating and slicing an intermediate combinedbytesobject while preserving identical output for any seed.Benchmark results:
1,000,000elements / run)LCGenerator_Key24_Val1000(seed+random_sample+randbytes(1024))1.640e-06 sec(2.23% std)3.653e-07 sec(3.18% std)-1,275 ns)SyntheticSourceGenKv_Key24_Val1000(_gen_kv_pair)1.815e-06 sec(5.45% std)4.266e-07 sec(4.40% std)-1,389 ns)SyntheticSourceRead_Key24_Val1000(source.read)1.828e-06 sec(0.47% std)7.581e-07 sec(5.03% std)-1,069 ns)