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

Hint the compiler that capacity is non-zero in mask_modulo #115

Closed
wants to merge 1 commit into from

Conversation

tertsdiepraam
Copy link
Contributor

@tertsdiepraam tertsdiepraam commented Jun 10, 2023

This is an alternative to #114. They are not compatible, sadly.

Modulo is faster for a non-zero divisor, so we can use the NonZeroUsize type to hint to the compiler that it can omit the case where the capacity is 0. However, we can only do that in non-const context, so we need separate functions for the ConstGenericRingBuffer and AllocRingBuffer. Which led me to implement it like this:

// Not const: used by AllocRingBuffer<T, NonPowerOfTwo>
#[inline]
fn mask_modulo(cap: usize, index: usize) -> usize {
    index % unsafe { NonZeroUsize::new_unchecked(cap) }
}

// Const: used by ConstGenericRingBuffer
#[inline]
const fn const_mask_modulo(cap: usize, index: usize) -> usize {
    index % cap
}

This theoretically should only impact AllocRingBuffer for non powers of two. The const_mask_modulo should compile to the same assembly as before, but who knows what the compiler might do. The benchmarks are a bit too noisy to tell.

The improvements for alloc non power of two are significant, but less significant than #114: it ranges from -10% to -30%. @jonay2000's more consistent benchmarks might help to determine the actual difference.

I'll leave it up to you what direction you want to go between this and #114 in (if any).

@tertsdiepraam tertsdiepraam marked this pull request as ready for review August 20, 2023 16:36
@jdonszelmann
Copy link
Collaborator

Closed in favor of #114

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