[connortsui20]

ZipEq::next has no #[inline], unlike Zip::next in core and ZipLongest::next in this crate.

Related: vortex-data/vortex#10962 (comment)

[phimuemue]

Hi there, could you provide some numbers about how much this would actually speed up your use case?

I ask because so far we do not have a policy regarding inline. If possible, I'd like to make an informed decision. As a side note, I somewhat assumed that the compiler would inline the call to next since the function body seems small enough... Maybe the panic prohibits inlining?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.61%. Comparing base (6814180) to head (fe0821f).
⚠️ Report is 225 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
- Coverage   94.38%   93.61%   -0.78%     
==========================================
  Files          48       52       +4     
  Lines        6665     6484     -181     
==========================================
- Hits         6291     6070     -221     
- Misses        374      414      +40     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[connortsui20]

So I was originally working with an LLM and it noticed that ZipEq::next was placed in a different CGU than where it was getting called from.

But the main reason I thought this was a good change was because Zip::next has #[inline] in core, so I thought this should be similar since it is supposed to be a drop-in replacement.

I ran some benchmarks on a VM (generated by an LLM, so again take this with a grain of salt):

Details

Adding #[inline] reduced elapsed time by 7–16% in Vortex’s nullable run-end Boolean decoding benchmarks with 16 codegen units. With one codegen unit, the same cases showed essentially no change.

For example, decoding 10,000 rows with alternating Boolean values and 90% valid runs:

Rows per run	Without annotation	With annotation	Time reduction
2	26.20 µs	22.28 µs	15.0%
10	5.704 µs	4.959 µs	13.1%

These are native wall-time measurements on a Xeon Platinum 8488C, using Rust 1.99.1 / LLVM 23.1.1, AVX2 flags, and Vortex’s bench profile with 16 CGUs and lto=false. The reductions are median paired ratios across 21 alternating AB/BA blocks. A few-runs control that bypasses zip_eq showed no clear change.

The assembly explains the difference: without the annotation, the decoder and the monomorphized ZipEq::next land in different codegen units, and the decoder calls next out of line once per run. Adding #[inline] eliminates those calls. With one CGU, the unannotated version already inlines, so the panic doesn’t prevent inlining outright.

The comparison uses Vortex at 0bd1d53c236e578d4a72428c6b24933f109834d3, before its zip workaround, with just this annotation backported to itertools 0.14. The next implementation is otherwise identical to the one here.

So TLDR; the issue is just that this can land in a different CGU without the inline hint. This is obviously only an issue if there are multiple CGUs.

[phimuemue]

Taking because of reasonably explained reasonable.

Thanks.