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

feat(byteview): Add non-consuming mmap constructor #448

Merged
merged 4 commits into from
Nov 18, 2021

Conversation

flub
Copy link
Contributor

@flub flub commented Nov 15, 2021

This adds constructor to mmap a File into a ByteView without consuming
the file object.

This adds constructor to mmap a File into a ByteView without consuming
the file object.
@flub flub requested review from untitaker and a team November 15, 2021 10:53
// Ensure we can still read from the the file after mmapping and deleting it on disk.
let mut buf = Vec::new();
file.rewind()?;
file.read_to_end(&mut buf)?;
Copy link
Member

Choose a reason for hiding this comment

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

So does mmap-ing keep the FD alive? Might be interesting to explicitly drop(file) after this, and see that the view still works (and holds on to the FD).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mmap only uses the fd to figure out what to map, afterwards you can close it or do whatever you like to it. I added the explicit drop to make it even more obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,

That "yes" is not really true as the rest of the sentence explains 😄 mmap does not use the fd at all other than finding the right file to map. so it doesn't keep the FD alive either.

@loewenheim
Copy link
Contributor

Since map_file only calls mmap_file internally anyway, is there a reason to keep both around? If we want to have both, I would propose naming them more distinctly; currently the names are very non-obvious and it's easy to read over the double m in mmap_file.

@flub
Copy link
Contributor Author

flub commented Nov 15, 2021

Since map_file only calls mmap_file internally anyway, is there a reason to keep both around? If we want to have both, I would propose naming them more distinctly; currently the names are very non-obvious and it's easy to read over the double m in mmap_file.

Yeah, the naming is not ideal and I'm open to better suggestions.

I think API-wise we can't really change the behaviour of the existing method or remove it right away though.

@flub
Copy link
Contributor Author

flub commented Nov 15, 2021

To be honest, I'm not overly concerned about the only naming difference being a single m even though that's easy to typo. The compiler will clearly make you chose the right one.

@jan-auer jan-auer requested a review from a team November 15, 2021 12:36
@flub flub enabled auto-merge (squash) November 18, 2021 09:39
@flub flub merged commit 62669a9 into master Nov 18, 2021
@flub flub deleted the feat/byteview-mmap-non-consuming branch November 18, 2021 09:43
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

4 participants