Dandandan commented on PR #23162: URL: https://github.com/apache/datafusion/pull/23162#issuecomment-4796609071
Thanks for the reviews @alamb @zhuqi-lucas! 🙏 @zhuqi-lucas — the dominant win is the **bounds-check reduction**; cache locality is a smaller, secondary effect. Some data points on `single_u64`: - inline the comparison + skip the redundant `maybe_poll_stream` (no cache): **~−12%** - + cache current/previous so the hot `compare`/`eq_to_previous` stop indexing the buffer: **~−15–18%** - an (unmerged) `unsafe` `get_unchecked` variant with no cache: **~−22%** The cache closes most of the gap to the `unsafe` version, and the disassembly confirms why: the primitive `poll_next` drops from 6 → 4 `panic_bounds_check` sites and **none** remain in `cursor.rs` (the per-comparison buffer checks are gone, and the once-per-row `set_offset` index is elided by the `offset < len` guard in `advance`). The 4 left are all `self.cursors[idx]` `Vec` accesses. Reading the cached fields instead of `buffer[offset]` does help locality too, but the measured recovery lines up with removing the checks. Also pushed a small follow-up trimming comment verbosity and replacing the `RowValues` note with the measured rationale (leaving its inline decision to the compiler — both `#[inline]` and `#[inline(never)]` regress the multi-column path). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
