Cleanup chunk_by count method and add tests

[hkBst]

View all comments

Cleanup chunk_by count method (introduced in #158866) to use array_windows for improved readability.

EDIT: also added tests for chunk_by.

[rustbot]

r? @Darksonn

rustbot has assigned @Darksonn.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[Darksonn]

It appears that this function is missing some tests.

View changes since this review

[hkBst]

@rustbot ready

[obi1kenobi]

Would you mind just verifying that the new implementation is still able to vectorize the count?

There was some sensitivity to that in the initial code. Unfortunately the existing perf suite doesn't measure this call site. My original PR used the omnibor crate to measure and hopefully you'd be able to do the same.

[Darksonn]

Good point, @obi1kenobi

@rustbot author

[hkBst]

All comments addressed, except I'm not able to test vectorization at this time. @obi1kenobi perhaps you would be willing to test that?

@rustbot ready

[obi1kenobi]

On my current x86-64 machine this is a perf regression predominantly for short inputs, both with and without vectorization.

Slice length	Native AVX2 slowdown	Generic x86-64 slowdown
2	16.4%	17.0%
8	9.3%	11.4%
16	14.9%	8.7%
64	3.6%	3.5%
8 KiB	~0–0.6%	~0.5%

Looking at the assembly, the proposed new code has extra branches and conditional-moves, plus a few extra arithmetic instructions. Here's a flavor of the extra instructions:

+	subq	$2, %rsi
+	cmovbq	%rdi, %rsi       # saturating array_windows length
...
+	incq	%rax             # tail bookkeeping
...
+	incq	%rdi             # add the initial chunk after reduction
+	movq	%rdi, %rax

I can include a full diff of the assembly if you'd like, but it's a bit noisy because registers got renamed.

I don't have my macOS machine handy at the moment (where I originally benchmarked this) but given the generated assembly is more complex, I expect directionally similar results. (For reference, #158866 was a ~2x win on macOS but a 4-5.8x win on this x86-64 machine, largely attributable to the macOS arm cores having more effective ILP in scalar mode than my x86-64 CPU).

I'm not a reviewer or decision-maker on this PR. But personally, I don't really find the new code any easier to read than the prior code. I think the extra tests are great though!

[Darksonn]

Given @obi1kenobi's analysis, I think we should keep the existing code. But I'd be happy to approve adding the tests.

[Darksonn]

Also, did you even change the code since this comment? #159389 (comment)