feat: build with stable rust #3881
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
a10y
commented
Jul 16, 2025
CodSpeed Performance ReportMerging #3881 will improve performances by 38.39%Comparing Summary
Benchmarks breakdown
|
robert3005
reviewed
Jul 16, 2025
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
a10y
commented
Jul 16, 2025
|
|
||
| /// Trait for all types which have a known upper-bound. | ||
| /// | ||
| /// Functions that receive a `TrustedLen` iterator can assume that it's `size_hint` is exact, |
Contributor
Author
There was a problem hiding this comment.
Technically this is a stricter bound than the std TrustedLen, which just guarantees that the upper bound is either Some(exact) or None if unknown. This is what we want for our extend_trusted though.
The nice part about defining our version of this trait is that we can leave extend_trusted as a safe method on BufferMut, which should never fail at runtime. If we ever wanna add a new case we just add a new impl here. But I don't expect many more if any.
a10y
commented
Jul 16, 2025
| macro_rules! impl_for_range { | ||
| ($($typ:ty),*) => { | ||
| $( | ||
| unsafe impl TrustedLen for std::ops::Range<$typ> {} |
Contributor
Author
There was a problem hiding this comment.
I can't just do impl<T> for Range<T> because they introduce a type-bound on another unstable TrustedStep trait...which is really just implemented for the integer types anyway
a10y
commented
Jul 16, 2025
a10y
commented
Jul 16, 2025
Comment on lines
+71
to
+73
| const _: () = { | ||
| assert!(size_of::<VortexError>() < 128); | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
One lint that popped off when I switched to stable was that apparently with all features activated, VortexError was a whopping 140 bytes, which was tripping the large_err check.
I boxed the Backtraces to keep the size down, now it's around ~88B. Still large but better.
Contributor
There was a problem hiding this comment.
we should aggressively trim it down
Contributor
Author
There was a problem hiding this comment.
Yes but this is not new, the lint is just new.
a10y
commented
Jul 16, 2025
Comment on lines
-1
to
-4
| // SPDX-License-Identifier: Apache-2.0 | ||
| // SPDX-FileCopyrightText: Copyright the Vortex contributors | ||
|
|
||
| #[rustversion::nightly] |
Contributor
Author
There was a problem hiding this comment.
This file is now superfluous, instead I opted to do what Tokio does when you wanna unlock unstable nightly-only things and just add a new cfg vortex_nightly that gates all of the portable_simd stuff.
To run it, you can use a nigthly compiler and pass the flags, e.g.
RUSTFLAGS="--cfg vortex_nightly" cargo +nightly build -p vortex-array --all-features
a10y
commented
Jul 16, 2025
| /// Returns an iterator over the buffer of elements of type T. | ||
| pub fn iter(&self) -> impl Iterator<Item = &T> + '_ { | ||
| self.as_slice().iter() | ||
| pub fn iter(&self) -> Iter<'_, T> { |
Contributor
Author
There was a problem hiding this comment.
I think this technically isn't a breaking change actually, b/c we're going from unnamed -> named type.
But the idea is that we have to impl TrustedLen for this, so you can BufferMut::extend_trusted from another Buffer.
So I just defined a concrete type that wraps std::slice::Iter and delegates the important methods to it.
a10y
commented
Jul 16, 2025
| } | ||
| } | ||
|
|
||
| impl<T> BufferMut<T> { |
Contributor
Author
There was a problem hiding this comment.
I didn't change any logic here, this is just moving methods around
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Contributor
Contributor
Benchmarks: TPC-H on NVMETable of Results
|
Contributor
Benchmarks: TPC-H on S3Table of Results
|
Contributor
Benchmarks: compressTable of Results
|
Contributor
Benchmarks: Clickbench on NVMETable of Results
|
Contributor
Author
|
Clickbench/TPC-H seem relatively unchanged, some things got up to 20% faster but I think that could just be noise. One of the compression benchmarks got 18% slower, I retriggered to see if that's real or not |
a10y
commented
Jul 16, 2025
Contributor
Author
|
It looks like that one slowdown persisted, I can't replicate it though (at least not on M1) develop: 
PR: 
|
Contributor
|
Is this one of the cases where we call extend instead of extendtrusted? |
Contributor
|
I found that the extend changes didn’t manifest on macOS but would on Linux |
Contributor
Author
|
I just ran the experiment again on a c5.12xlarge and the results were similar to mac, if anything slightly faster on the PR branch 
|
Contributor
|
Ok, then let’s assume this a flake and move on |
robert3005
reviewed
Jul 16, 2025
| # vortex-duckdb-ext currently fails linking for cargo test targets. | ||
| run: | | ||
| cargo nextest run --locked --workspace --all-features --no-fail-fast --target x86_64-unknown-linux-gnu | ||
| cargo +nightly nextest run --locked --workspace --all-features --no-fail-fast --target x86_64-unknown-linux-gnu |
Contributor
There was a problem hiding this comment.
interesting that this uses nightly, I thought nextest would be fine on stable
Contributor
Author
There was a problem hiding this comment.
This is only for the Rust testing with ASAN job. That requires unstable -Z sanitizer flag
robert3005
approved these changes
Jul 16, 2025
Contributor
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
part of me thinks we shouldn't raise MSRV but part of me thinks maybe we can?
Contributor
Author
|
I think we can probably bump it whenever we want/whenever we need new features |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3546
MSRV is 1.87.0.
Stable is far ahead of the February 2025 nightly we've been pegged to, so on top of the regular changes there were a ton of new lints to go whack-a-mole.
I left rustfmt with its unstable options, which means that you'll need to continue to run rustfmt on the nightly channel but the project will be on the stable channel. You can run formatting from CLI like this
For RustRover you can configure rustfmt to execute with nightly in settings:
For VSCode and others you'll need to figure that one out.
Signed-off-by: Andrew Duffy andrew@a10y.dev