Optimize Ref<_, T> methods when T: Sized (#2752) #2758

bmberger · 2025-11-08T00:21:27Z

Benchmarks are surprisingly showing minimal impact...

With optimization:
test r#ref::tests::bench_deref_mut_sized ... bench: 0.23 ns/iter (+/- 0.03)
test r#ref::tests::bench_deref_sized ... bench: 0.44 ns/iter (+/- 0.04)
test r#ref::tests::bench_from_bytes_sized ... bench: 4.64 ns/iter (+/- 0.03)
test r#ref::tests::bench_into_mut_sized ... bench: 0.23 ns/iter (+/- 0.03)
test r#ref::tests::bench_into_ref_sized ... bench: 0.44 ns/iter (+/- 0.00)

Without optimization:
test r#ref::tests::bench_deref_mut_sized ... bench: 0.22 ns/iter (+/- 0.02)
test r#ref::tests::bench_deref_sized ... bench: 0.44 ns/iter (+/- 0.03)
test r#ref::tests::bench_from_bytes_sized ... bench: 4.66 ns/iter (+/- 0.31)
test r#ref::tests::bench_into_mut_sized ... bench: 0.23 ns/iter (+/- 0.03)
test r#ref::tests::bench_into_ref_sized ... bench: 0.44 ns/iter (+/- 0.01)

Closes #2752

gemini-code-assist · 2025-11-08T00:21:41Z

Summary of Changes

Hello @bmberger, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces targeted optimizations for Ref methods within the zerocopy crate when dealing with Sized types. The goal is to improve performance by allowing direct pointer manipulation, avoiding unnecessary intermediate conversions. While initial benchmarks show only minimal performance changes, the update refines the internal handling of Sized types and includes new benchmarks to monitor these specific operations.

Highlights

Optimization for Sized Types: Implemented a specialized code path for Ref<_, T> methods (into_ref, into_mut, deref, deref_mut) when the generic type T is Sized.
Direct Pointer Manipulation: This optimization bypasses intermediate ByteSlice conversions by directly using Ptr for Sized types, aiming for improved efficiency.
New Benchmarks: Added dedicated benchmark tests to measure the performance of these optimized Ref methods specifically for Sized types.
Test Feature Enablement: Enabled the test feature in src/lib.rs to support the new benchmarks.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces an optimization for Sized types in several methods of Ref<_, T>. The logic seems sound, and the addition of benchmarks is a great way to validate the performance impact. My main feedback is regarding significant code duplication across four methods, which could be refactored to improve maintainability. I've provided a suggestion to use a macro to deduplicate this logic.

src/ref.rs

Benchmarks are surprisingly showing minimal impact... With optimization: test r#ref::tests::bench_deref_mut_sized ... bench: 0.72 ns/iter (+/- 0.09) test r#ref::tests::bench_deref_sized ... bench: 0.70 ns/iter (+/- 0.09) test r#ref::tests::bench_from_bytes_sized ... bench: 5.27 ns/iter (+/- 0.69) test r#ref::tests::bench_into_mut_sized ... bench: 0.70 ns/iter (+/- 0.07) test r#ref::tests::bench_into_ref_sized ... bench: 0.94 ns/iter (+/- 0.08) Without optimization: test r#ref::tests::bench_deref_mut_sized ... bench: 0.72 ns/iter (+/- 0.23) test r#ref::tests::bench_deref_sized ... bench: 0.73 ns/iter (+/- 0.09) test r#ref::tests::bench_from_bytes_sized ... bench: 5.26 ns/iter (+/- 0.69) test r#ref::tests::bench_into_mut_sized ... bench: 0.70 ns/iter (+/- 0.08) test r#ref::tests::bench_into_ref_sized ... bench: 0.97 ns/iter (+/- 0.08) Closes google#2752 Co-authored-by: Joshua Liebow-Feeser <[email protected]>

joshlf · 2025-12-08T20:01:26Z

src/lib.rs

    __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS,
    feature(layout_for_ptr, coverage_attribute)
 )]
+#![cfg_attr(test, feature(test))]


Suggested change

#![cfg_attr(test, feature(test))]

#![cfg_attr(all(test, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS), feature(test))]

This ensures that we only compile benchmarks on the nightly compiler, which is required since feature(test) is an unstable (ie, nightly-only) Rust feature.

joshlf · 2025-12-08T20:01:37Z

src/lib.rs

 #[cfg(any(feature = "derive", test))]
 extern crate self as zerocopy;

+#[cfg(test)]


Suggested change

#[cfg(test)]

#[cfg(all(test, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS))]

joshlf · 2025-12-08T20:03:49Z

src/ref.rs

+                (
+                    BecauseMutationCompatible,
+                    (
+                        BecauseRead,
+                        (BecauseExclusive, BecauseExclusive),
+                    ),
+                ),


You should be able to replace some of these with underscores (at the most extreme, (_, (_, (_, _)))) and inference will still work – that'll make this more concise and format more nicely.

joshlf · 2025-12-08T20:03:59Z

src/ref.rs

+                (
+                    BecauseMutationCompatible,
+                    (
+                        BecauseRead,
+                        (BecauseExclusive, BecauseExclusive),
+                    ),
+                ),


Same here re: underscores

joshlf · 2025-12-08T20:04:06Z

src/ref.rs

+                (
+                    BecauseMutationCompatible,
+                    (
+                        BecauseRead,
+                        (BecauseExclusive, BecauseExclusive),
+                    ),
+                ),


Same here re: underscores

joshlf · 2025-12-08T20:08:12Z

src/ref.rs

+                    // SAFETY: The safety invariants of `Ptr::new` (see definition) are
+                    // satisfied:
+                    // 0. If `ptr`'s referent is not zero sized, then `ptr` has valid
+                    //    provenance for its referent, because it derived from `self`
+                    //    using a series of provenance-preserving operations, and
+                    //    because `self` has valid provenance for its referent. By the
+                    //    same argument, `ptr`'s referent is entirely contained within
+                    //    the same allocated object as `self`'s referent.
+                    // 1. If `ptr`'s referent is not zero sized, then the allocation of
+                    //    `ptr` is guaranteed to live for at least `'a`, because `ptr`
+                    //    is entirely contained in `self`, which lives for at least `'a`
+                    //    by invariant on `Ptr`.


Same as above

joshlf · 2025-12-08T20:08:20Z

src/ref.rs

+                    let ptr = T::raw_from_ptr_len(
+                        ptr.as_non_null().cast(),
+                        <T::PointerMetadata as crate::PointerMetadata>::from_elem_count(0),
+                    );


Suggested change

let ptr = T::raw_from_ptr_len(

ptr.as_non_null().cast(),

<T::PointerMetadata as crate::PointerMetadata>::from_elem_count(0),

);

let p = T::raw_from_ptr_len(

ptr.as_non_null().cast(),

<T::PointerMetadata as crate::PointerMetadata>::from_elem_count(0),

);

joshlf · 2025-12-08T20:08:25Z

src/ref.rs

+                    // SAFETY: The safety invariants of `Ptr::new` (see definition) are
+                    // satisfied:
+                    // 0. If `ptr`'s referent is not zero sized, then `ptr` has valid
+                    //    provenance for its referent, because it derived from `self`
+                    //    using a series of provenance-preserving operations, and
+                    //    because `self` has valid provenance for its referent. By the
+                    //    same argument, `ptr`'s referent is entirely contained within
+                    //    the same allocated object as `self`'s referent.
+                    // 1. If `ptr`'s referent is not zero sized, then the allocation of
+                    //    `ptr` is guaranteed to live for at least `'a`, because `ptr`
+                    //    is entirely contained in `self`, which lives for at least `'a`
+                    //    by invariant on `Ptr`.


Same as above

joshlf · 2025-12-08T20:09:19Z

src/ref.rs

+    #[bench]
+    fn bench_from_bytes_sized(b: &mut Bencher) {
+        let buf = Align::<[u8; 8], AU64>::default();
+        // `buf.t` should be aligned to 8, so this should always succeed.
+        let bytes = &buf.t[..];
+        b.iter(|| test::black_box(Ref::<_, AU64>::from_bytes(test::black_box(bytes)).unwrap()));
+    }
+
+    #[bench]
+    fn bench_into_ref_sized(b: &mut Bencher) {
+        let buf = Align::<[u8; 8], AU64>::default();
+        let bytes = &buf.t[..];
+        let r = Ref::<_, AU64>::from_bytes(bytes).unwrap();
+        b.iter(|| {
+            test::black_box(Ref::into_ref(test::black_box(r)))
+        });
+    }
+
+    #[bench]
+    fn bench_into_mut_sized(b: &mut Bencher) {
+        let mut buf = Align::<[u8; 8], AU64>::default();
+        let _ = Ref::<_, AU64>::from_bytes(&mut buf.t[..]).unwrap();
+        b.iter(move || {
+            // SAFETY: The preceding `from_bytes` succeeded, and so we know that
+            // `&mut buf.t[.]   
+            let r = unsafe { Ref::<&mut [u8], AU64>::new_unchecked(&mut buf.t[..]) };
+            test::black_box(Ref::into_mut(test::black_box(r)));
+        });
+    }
+
+    #[bench]
+    fn bench_deref_sized(b: &mut Bencher) {
+        let buf = Align::<[u8; 8], AU64>::default();
+        let bytes = &buf.t[..];
+        let r = Ref::<_, AU64>::from_bytes(bytes).unwrap();
+        b.iter(|| {
+            let temp = test::black_box(r);
+            test::black_box(temp.deref());
+      });
+    }
+
+    #[bench]
+    fn bench_deref_mut_sized(b: &mut Bencher) {
+        let mut buf = Align::<[u8; 8], AU64>::default();
+        let _ = Ref::<_, AU64>::from_bytes(&mut buf.t[..]).unwrap();
+        b.iter(|| {
+            // SAFETY: The preceding `from_bytes` succeeded, and so we know that
+            // `&mut buf.t[.]            
+            let r = unsafe { Ref::<&mut [u8], AU64>::new_unchecked(&mut buf.t[..]) };
+            let mut temp = test::black_box(r);
+            test::black_box(temp.deref_mut());
+        });
+    }


Let's move these into a new #[cfg(all(test, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS))] mod benches below mod tests?

joshlf · 2025-12-08T20:11:31Z

src/ref.rs

+            let ptr: Ptr<'_, T, (Shared, Unaligned, Initialized)> = unsafe {
+                ptr.cast_unsized(|ptr| {
+                    let ptr = T::raw_from_ptr_len(
+                        ptr.as_non_null().cast(),
+                        <T::PointerMetadata as crate::PointerMetadata>::from_elem_count(0),
+                    );
+                    // SAFETY: The safety invariants of `Ptr::new` (see definition) are
+                    // satisfied:
+                    // 0. If `ptr`'s referent is not zero sized, then `ptr` has valid
+                    //    provenance for its referent, because it derived from `self`
+                    //    using a series of provenance-preserving operations, and
+                    //    because `self` has valid provenance for its referent. By the
+                    //    same argument, `ptr`'s referent is entirely contained within
+                    //    the same allocated object as `self`'s referent.
+                    // 1. If `ptr`'s referent is not zero sized, then the allocation of
+                    //    `ptr` is guaranteed to live for at least `'a`, because `ptr`
+                    //    is entirely contained in `self`, which lives for at least `'a`
+                    //    by invariant on `Ptr`.
+                    unsafe { PtrInner::new(ptr) }
+                })
+            };


You should be able to factor this entire thing out into a private, unsafe bare function below with its own separate safety preconditions. If you make it generic over the aliasing type parameter, you should be able to have a single function that's used by all four methods.

gemini-code-assist bot reviewed Nov 8, 2025

View reviewed changes

src/ref.rs Show resolved Hide resolved

joshlf requested changes Nov 8, 2025

View reviewed changes

src/ref.rs Outdated Show resolved Hide resolved

joshlf requested changes Nov 9, 2025

View reviewed changes

src/ref.rs Show resolved Hide resolved

src/ref.rs Show resolved Hide resolved

src/ref.rs Show resolved Hide resolved

src/ref.rs Outdated Show resolved Hide resolved

joshlf requested changes Nov 13, 2025

View reviewed changes

src/ref.rs Show resolved Hide resolved

src/ref.rs Outdated Show resolved Hide resolved

src/ref.rs Outdated Show resolved Hide resolved

src/ref.rs Show resolved Hide resolved

bmberger force-pushed the optimize-ref-methods branch from 3960f56 to f68da63 Compare November 14, 2025 23:18

bmberger force-pushed the optimize-ref-methods branch from f68da63 to 08ba7b2 Compare November 14, 2025 23:20

bmberger requested a review from joshlf November 14, 2025 23:23

joshlf requested changes Dec 8, 2025

View reviewed changes

	#![cfg_attr(test, feature(test))]
	#![cfg_attr(all(test, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS), feature(test))]

	#[cfg(test)]
	#[cfg(all(test, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS))]

Optimize Ref<_, T> methods when T: Sized (#2752) #2758

Are you sure you want to change the base?

Optimize Ref<_, T> methods when T: Sized (#2752) #2758

Conversation

bmberger commented Nov 8, 2025

Uh oh!

gemini-code-assist bot commented Nov 8, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants