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

Remove unneeded unsafe in data_directories.rs #261

Merged
merged 1 commit into from Feb 15, 2021

Conversation

nico-abram
Copy link
Contributor

This pull request replaces get_unchecked calls with direct indexing ( [] ). This removes a bunch of unsafe blocks

As far as I can tell, this should not affect optimized code: Since rust statically knows the size of the array, it should be able to elide the bounds checks

This godbolt link shows how both the version with and without the unsafe are compiled to the same function with opt-level=3. This is the same example without the derives adding noise to the output

What I found interesting is, according to this godbolt link when compiling without optimizations, rust also elides the bounds checks for the indexing version, and the get_unchecked version has to call the get_unchecked function which does not get inlined
imagen

So I would expect the new version to outperform the unsafe version on debug builds, and match the unsafe version with optimizations

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

@nico-abram thank you so much for doing the legwork here, including checking assembly outputs; this has come up before in context of a a "safety audit" (which never materialized), but i'm glad to see the research done to see that switching to regular indexing has no perf regression (this was stated generally, without evidence). Much appreciated!

@m4b m4b merged commit 578c790 into m4b:master Feb 15, 2021
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.

None yet

2 participants