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

Documentation, naive_count_32 vs naive_count #71

Closed
ndmitchell opened this issue Jul 1, 2021 · 5 comments
Closed

Documentation, naive_count_32 vs naive_count #71

ndmitchell opened this issue Jul 1, 2021 · 5 comments

Comments

@ndmitchell
Copy link

These two functions are presented, with naive_count_32 having the weakness that it can overflow. That's a trade-off I might be happy to make, but the documentation doesn't say what the performance difference is. Could that be added, at least in one benchmark, to enable a more informed decision?

@Veedrac
Copy link
Collaborator

Veedrac commented Jul 1, 2021

Per a cargo bench run, naive_count_32 can be some three times as fast from 40 characters upward, due to better vectorization.

I'm not sure I actually want to document this, since performance is so temperamental given its reliance on compiler behaviours of the month.

@ndmitchell
Copy link
Author

Without that information, how can I be expected to make a choice between the two alternatives? Short of running my own benchmarks, and that's way too much effort in most cases. Just having that one data point gives me good information.

@llogiq
Copy link
Owner

llogiq commented Jul 2, 2021

As @Veedrac said, this pretty much relies on compiler behavior, which can be a fickle beast. So you should run your own benchmarks before deciding to risk overflow for unknown gains.

@LoganDark
Copy link

So you should run your own benchmarks before deciding to risk overflow for unknown gains.

It's worth adding that, should you go this route, you need to also re-run the benchmarks each compiler update to track regressions. It is a lot of effort to rely on the optimizer in a safe and reliable way.

@Veedrac
Copy link
Collaborator

Veedrac commented Apr 4, 2022

Closing wontfix. I am sympathetic to wanting guidance in the documentation, but in this case, if you haven't benchmarked your code as integrated, then I would be much more comfortable if you just use the one that is more correct for the context.

@Veedrac Veedrac closed this as completed Apr 4, 2022
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

No branches or pull requests

4 participants