Skip to content

Deduplicate BitBuffer set-index visitors - #9461

Merged
connortsui20 merged 1 commit into
developfrom
ct/bit-buffer-infallible-visitor
Aug 18, 2026
Merged

Deduplicate BitBuffer set-index visitors#9461
connortsui20 merged 1 commit into
developfrom
ct/bit-buffer-infallible-visitor

Conversation

@connortsui20

@connortsui20 connortsui20 commented Aug 18, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Keeps the infallible and fallible set-index visitors on one implementation. Optimized LLVM IR and assembly for the measured hot path were identical. Eight paired runtime comparisons on macOS with Apple M4 Max and rustc 1.97.1 showed no clear difference.

What changes are included in this PR?

Routes BitBuffer::for_each_set_index through try_for_each_set_index with Infallible, removing the duplicate traversal. The vortex-buffer tests, all-target/all-feature Clippy, and doctests pass.

What APIs are changed? Are there any user-facing changes?

No public API or behavior changes.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/chore A trivial change label Aug 18, 2026
@connortsui20
connortsui20 marked this pull request as ready for review August 18, 2026 18:19
@connortsui20
connortsui20 marked this pull request as draft August 18, 2026 18:20
@connortsui20
connortsui20 marked this pull request as ready for review August 18, 2026 18:20
@codspeed-hq

codspeed-hq Bot commented Aug 18, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.88%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 1 regressed benchmark
✅ 1999 untouched benchmarks
🆕 4 new benchmarks
⏩ 89 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_canonicalize_into[(1000, 50, 8, 64)] 15.1 ms 16.9 ms -10.88%
🆕 Simulation all_valid_exclusive[4096] N/A 482.6 µs N/A
🆕 Simulation all_valid_exclusive[65536] N/A 7.3 ms N/A
🆕 Simulation nullable_exclusive[4096] N/A 358.7 µs N/A
🆕 Simulation nullable_exclusive[65536] N/A 5.2 ms N/A

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/bit-buffer-infallible-visitor (d67c41c) with develop (bf6b2be)2

Open in CodSpeed

Footnotes

  1. 89 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on develop (004e512) during the generation of this report, so bf6b2be was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@connortsui20

connortsui20 commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

this came up from #9353 (comment)

@connortsui20

Copy link
Copy Markdown
Member Author

This one looks good. Rechecked 3805421 against d67c41c on x86-64 with AVX2. The direct for_each_set_index benchmark keeps the same optimized vector loop and emits identical assembly. The reported FSST benchmark does not instantiate this path and its benchmark root is unchanged.

Direct `for_each_set_index` LLVM IR and assembly

Configuration:

  • rustc 1.97.1 with LLVM 22.1.6
  • profile.bench: 16 CGUs, LTO disabled
  • x86_64-apple-darwin with +avx2
  • baseline 3805421bd68ec8104131e4fb9c73551fbebc94ca
  • candidate d67c41cb24e4219250bdb0fb469c0cd92f3d06ac

The final monomorphization comes from vortex-mask/benches/mask_iteration.rs::for_each_set_index:

CARGO_TARGET_DIR=<separate-parent-or-head-target> \
RUSTC_WRAPPER= \
RUSTFLAGS='-C target-feature=+avx2 -C force-frame-pointers=yes -C remark=loop-vectorize -C debuginfo=line-tables-only' \
cargo rustc -p vortex-mask --bench mask_iteration --profile bench \
  --target x86_64-apple-darwin -- --emit=llvm-ir,asm,link

Both revisions retain the same <4 x i64> vector body with four-way interleaving. The candidate IR contains no surviving Result, Err, Infallible, or try_for_each_set_index control flow. The executable LLVM opcode inventories are identical:

42 add             19 and             74 br
66 call            75 getelementptr   41 icmp
66 load            96 phi             19 shl
11 sub             14 zext

The complete inventory SHA-256 is 2f8fb56bd534775ef1753f922eadaa7fe6d8089a65bd7ad260b0debdc5df9d9e for both revisions.

The bounded pre-link assembly body has 723 instructions on both revisions and is byte-for-byte identical after removing line-table directives. Its SHA-256 is ee03d9803a1d423e580ff3a6e7ad16d899b8b09d3ac07e953d54085b25266580 for both revisions.

Reported FSST CodSpeed regression

The reported case was chunked_canonicalize_into[(1000,50,8,64)]. The emitted benchmark IR contains no for_each_set_index monomorphization.

The optimized benchmark root has the same normalized LLVM IR SHA-256 on both revisions:

c52929eb2196f4fa3c88d3738c2a8843755edd3f80f2d5558b86622d711b148b

Its bounded pre-link assembly body has 446 instructions on both revisions and is byte-for-byte identical. The normalized assembly SHA-256 is:

ed9348634bddcf88466cdca00589a00ea5f962979da1febeeda60ea0bd9c9399

CodSpeed also compared against fallback develop commit bf6b2be, rather than the PR base 004e512. The reported movement is not attributable to this PR.

This is compiler-output evidence, not wall-clock evidence. The target matches CodSpeed's x86-64 architecture and AVX2 feature set, but uses the macOS ABI rather than the Linux ABI.

@connortsui20
connortsui20 merged commit 54e01d4 into develop Aug 18, 2026
96 of 99 checks passed
@connortsui20
connortsui20 deleted the ct/bit-buffer-infallible-visitor branch August 18, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants