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

investigate generic sample_indicies #202

Closed
vitiral opened this issue Dec 5, 2017 · 2 comments
Closed

investigate generic sample_indicies #202

vitiral opened this issue Dec 5, 2017 · 2 comments

Comments

@vitiral
Copy link
Contributor

vitiral commented Dec 5, 2017

This is related to several comments in #201 and is an extension of the work done as part of #194.

In many cases sample_indicies can be a u32 or even a u16 (u8 is probably not worth it) to save on performance and memory usage. This applies to both the cache and inplace implementations. This ticket is to investigate what it would take to make the function generic.

This should not be (much of) a breaking change, since in most cases type inference would infer the usize requirement. However, it would be a potentially minor one.

@vitiral
Copy link
Contributor Author

vitiral commented Dec 5, 2017

note: it's going to be a little while until I get the chance to dig into this, so if anyone else is interested then have at it!

@dhardy
Copy link
Member

dhardy commented Jun 1, 2018

Using u32 in place of usize (aka u64) improves performance of the inplace impl significantly. Since this method is inappropriate for length > ~1e7 and u32 suffices up to ~4e9 I see no point having a u64 version.

Using u16 for inplace improves performance up to 3x in benchmarks (10 of 10'000 case) but normally much less than 2x, and sometimes worse. Since the algorithm is already quite fast for these representable lengths (around 10ms for length 10'000) I don't see a big incentive to investigate further (also, these benchmarks give some strange results).

The cache method appears to be about 10% faster using u32.

For completeness, I also tested Floyd's alg (#479) with u32, and it is around 3-15% faster.

For conciseness, I think the best approach is to make the inplace method use u32 only and keep the other methods using usize (since the latter are both applicable to sample a small "amount" from a huge range; that said I'm not sure how often they would be used for this).

Note that I don't think generics are a great option here, since users will often use usize even with ranges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants