Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert the StableVec class to use virtual addressing #3608

Merged

Conversation

jason-nitro
Copy link

@jason-nitro jason-nitro commented Nov 13, 2024

Problem

When doing a 32-bit build of Agave, pointers are of course 32 bits. The StableVec class is used to store vectors within the interpreter's 64-bit address space, but uses a Rust pointer to point to its data (and usize for both cap and len fields).

Summary of Changes

This PR updates the StableVec to use u64s for virtual addresses and sizes, rather than the compilers pointer and usize type, which are 32 bits in a 32-bit build, even though they contain 64-bit virtual addresses.

This could lead to an issue if this class were used to copy vectors between physical and virtual space (e.g. a StableVec that points to data in virtual memory, but is copied out into a Rust vector that now stuffs everything into 32-bit pointers and usizes), but at worst that would only be an issue for 32-bit builds, not the usual 64-bit ones, and that also does not appear to be something that actually happens. StableVec is used to store vectors in situations where the exact memory layout cannot change, i.e. inside the vm space, not as an kind of enhanced version of Rust's usual Vector class.

Fixes #

@mergify mergify bot requested a review from a team November 13, 2024 14:48
Copy link

mergify bot commented Nov 13, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@jason-nitro
Copy link
Author

@alessandrod @Lichtso 32-bit build fixes (PR 2 of 2, so far)

@Lichtso Lichtso added the CI Pull Request is ready to enter CI label Dec 31, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 31, 2024
pub ptr: NonNull<T>,
pub cap: usize,
pub len: usize,
pub addr: u64,
Copy link

Choose a reason for hiding this comment

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

ptr was renamed to addr here but forgotten to change it in test_memory_layout() as well.

@jason-nitro jason-nitro requested a review from a team as a code owner January 9, 2025 15:04
@Lichtso Lichtso added the CI Pull Request is ready to enter CI label Jan 9, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 9, 2025
@LucasSte
Copy link

This PR also needs a rebase for CI to pass.

@jason-nitro jason-nitro force-pushed the jdavis_stablevec_virtual_addressing branch from 6138fdd to d947b82 Compare January 10, 2025 17:28
@jason-nitro jason-nitro force-pushed the jdavis_stablevec_virtual_addressing branch from d947b82 to b2a34d7 Compare January 15, 2025 18:58
@Lichtso Lichtso added the CI Pull Request is ready to enter CI label Jan 16, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 16, 2025
@@ -56,13 +55,13 @@ impl<T> StableVec<T> {

impl<T> AsRef<[T]> for StableVec<T> {
fn as_ref(&self) -> &[T] {
self
self.deref()
Copy link

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

.. At this point, I honestly don't recall other than that's what makes the types correct. Maybe previously it was constructing a true slice directly from the data (evilly), and now it's using deref to do the from_raw_parts thing..?

Copy link

Choose a reason for hiding this comment

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

Well, it is a bit more explicit now, so that probably won't hurt. Was just curious if there was another reason behind it.

@Lichtso Lichtso merged commit 43cdbb4 into anza-xyz:master Jan 22, 2025
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants