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

Make filled_with more generic and recover from panics better. #4

Conversation

jswrenn
Copy link
Contributor

@jswrenn jswrenn commented Aug 3, 2019

More generic initializer.

The initializer is now an F: FnMut() -> T, allowing, for example, using a stateful closure to initialize the vec.

Better panic recovery.

If the initializer of filled_with panics, items pushed prior to the panic will be dropped.

Copy link
Owner

@slightlyoutofphase slightlyoutofphase left a comment

Choose a reason for hiding this comment

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

Sorry it took me a bit to respond! Needed a little more time to consider than the other PR. Your idea is great WRT to the panic stuff, and using a FnMut instead of a fn instead is definitely more useful as far as what it can take as input.

That said, after looking at some assembler output, your code as written seems to inhibit the auto-vectorization of the initialization loop even in the best case scenario (release mode, no panic actually happens, etc.)

I played around with it a bit though, and found that the following has all of your intended benefits as far as dropping the items when things do go wrong, while also not preventing optimizations when things go fine:

///Returns a new StaticVec instance filled with the return value of an initializer function.
///The length field of the newly created StaticVec will be equal to its capacity.
///
///```rust
///use staticvec::*;
///
///fn main() {
///  let mut i = 0;
///  let v = StaticVec::<i32, 64>::filled_with(|| { i += 1; i });
///
///  assert_eq!(v.len(), 64);
///
///  assert_eq!(v[0], 1);
///  assert_eq!(v[1], 2);
///  assert_eq!(v[2], 3);
///  assert_eq!(v[3], 4);
///}
/// ```
#[inline]
pub fn filled_with<F>(mut initializer: F) -> Self
where F: FnMut() -> T {
  unsafe {
    let mut res = Self {
      data: MaybeUninit::uninit().assume_init(),
      length: 0,
    };
    for i in 0..N {
      res.data.get_unchecked_mut(i).write(initializer());
      res.length += 1;
    }
    res
  }
}

If you want to switch it out in your branch for that (so you can still get "credit" for the commit), I'm happy to merge it right away.

GENERIC INITIALIZER:
The initializer is now an `F: FnMut() -> T`, allowing, for example,
using a stateful closure to initialize the vec.

PANIC RECOVERY:
If the initializer of `filled_with` panics, items pushed prior to
the panic will be dropped.
@jswrenn jswrenn force-pushed the filled_with-improvements branch from dea44d1 to 1119f52 Compare August 3, 2019 22:20
@jswrenn
Copy link
Contributor Author

jswrenn commented Aug 3, 2019

I've amended the PR! You're right, push_unchecked essentially needs to be manually inlined to get auto vectorization. It looks like calling Self::new() vs inlining it doesn't make a difference, however.

@slightlyoutofphase slightlyoutofphase merged commit 046091d into slightlyoutofphase:master Aug 4, 2019
@slightlyoutofphase
Copy link
Owner

Sold to the highest bidder! Or, merged.

(This kinda thing is why people who say "never ever ever use #[inline(always)]!" are straight up wrong, I'll note.)

@jswrenn
Copy link
Contributor Author

jswrenn commented Aug 4, 2019

Huzzah!

It's also an example of why relying on inline(always) to ensure an abstraction is zero-cost might fall flat. :(

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Aug 4, 2019

It's also an example of why relying on inline(always) to ensure an abstraction is zero-cost might fall flat. :(

For sure. That's basically why I always use --emit asm to compare the output after making any change that might unintentionally throw off the optimizer.

Thanks again for the contribution BTW, I probably wouldn't have ever thought to use FnMut or improve the panic handling on my own.

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