Skip to content

RFC: Remove aligned-vec dependency#51

Merged
shssoichiro merged 4 commits intorust-av:mainfrom
FreezyLemon:remove-aligned-vec-dep
Apr 16, 2026
Merged

RFC: Remove aligned-vec dependency#51
shssoichiro merged 4 commits intorust-av:mainfrom
FreezyLemon:remove-aligned-vec-dep

Conversation

@FreezyLemon
Copy link
Copy Markdown
Contributor

Motivation:

  • DIY version is small (<200 LoC vs 1500). normally not a big deal but there is a very high amount of unsafe code in the crate
  • crate seems to be passively maintained (two issues regarding UB have been open for a while without response)1
  • slims down dependency tree a bit

Footnotes

  1. cases of UB should not concern v_frame

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 95.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.48%. Comparing base (f629ccb) to head (d53a34f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/plane/aligned.rs 95.23% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   94.27%   94.48%   +0.20%     
==========================================
  Files           3        4       +1     
  Lines         332      453     +121     
==========================================
+ Hits          313      428     +115     
- Misses         19       25       +6     

☔ View full report in Codecov by Sentry.
📢 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.

@FreezyLemon FreezyLemon force-pushed the remove-aligned-vec-dep branch from 2df74a5 to d05f143 Compare April 13, 2026 19:23
@FreezyLemon
Copy link
Copy Markdown
Contributor Author

Of course the performance regresses now... let me investigate.

@FreezyLemon FreezyLemon marked this pull request as draft April 13, 2026 19:35
also seal it so downstream can't implement it
This lets us drop our dependency on aligned-vec.
@FreezyLemon FreezyLemon force-pushed the remove-aligned-vec-dep branch from d05f143 to 80b1585 Compare April 16, 2026 07:14
@FreezyLemon FreezyLemon marked this pull request as ready for review April 16, 2026 09:14
@FreezyLemon
Copy link
Copy Markdown
Contributor Author

FreezyLemon commented Apr 16, 2026

Okay, ready now. If we include #52 then there is no performance regression on my machine. byte_data seems a fair bit quicker but still comparatively slow, might be worth investigating afterwards.

As a bonus, AlignedData allows access to uninitialized data in a UB-free manner through MaybeUninit, if we want to expose this through the public API later.

@FreezyLemon FreezyLemon force-pushed the remove-aligned-vec-dep branch from 80b1585 to d53a34f Compare April 16, 2026 12:32
Comment thread src/plane/aligned.rs
);

for (new, old) in new.iter_mut().zip(self.iter()) {
new.write(old.clone());
Copy link
Copy Markdown
Member

@shssoichiro shssoichiro Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can make a specialized impl for T: Copy that uses copy_nonoverlapping (or copy_from_slice) instead. Should be even faster if your other PR is any indication.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speed seems to be about the same on my machine for a Plane::clone() vs a plane.copy_from_slice(&data) from the benches. Roughly 35-40 microseconds, so I think the compiler already catches it in this case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to not be guaranteed. There may just not be any places using it in a performance critical way in this crate. Nevertheless since I'm already testing it, I may as well push up a specialization for it.

Iterator method generates a loop:

.LBB3_5:
	movzx edx, byte ptr [r14 + rcx]
	mov byte ptr [rax + rcx], dl
	inc rcx
	cmp rbx, rcx
	jne .LBB3_5
	jmp .LBB3_6
.LBB3_1:
	mov eax, 1

copy_nonoverlapping generating a memcpy:

.LBB3_1:
	mov edi, 1
.LBB3_4:
	mov rsi, r14
	mov rdx, rbx
	mov r14, rdi
	call qword ptr [rip + memcpy@GOTPCREL]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently specialization is still not stable. WTF Rust it's been like 8 years.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you had some neat trick up your sleeve 🥲
I think specialization is still some time away too.. would've been nice to have, but oh well

@shssoichiro shssoichiro merged commit 3c31dbb into rust-av:main Apr 16, 2026
9 checks passed
@FreezyLemon FreezyLemon deleted the remove-aligned-vec-dep branch April 16, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants