Use mmap_mode for probing#86
Conversation
|
Hey @nh2, thanks for the patch! This looks pretty simple, so it shouldn't take long to review and merge. By the way, the documentation preview action failed because your branch was made from an earlier release, and the latest commits on master are needed for readthedocs.org to do the build. You don't need to do anything about this. |
dranjan
left a comment
dranjan
left a comment
There was a problem hiding this comment.
The changes make sense to me.
Besides my one comment about the docstring, the only other thing I can think of at the moment is that it would be nice to add a test for this bug fix, but I can't immediately think of how. I guess we would need to make a file object that's readable and memory-mappable but can't be memory-mapped in copy-on-write mode, but if we need to trigger the memory overcommitment issue, I don't think we can do that in a unit test. If you can think of a solution, do let me know. However, to be clear, this isn't blocking the pull request.
If you want to try to address the docstring thing, let me know. Otherwise, I'll probably make the changes and merge later this week and then push a point release.
| # Loading the data is straightforward. We will memory | ||
| # map the file in copy-on-write mode. | ||
| mmap_mode = mmap if isinstance(mmap, str) else 'c' | ||
| # map the file in the requested mode. |
There was a problem hiding this comment.
This comment was wrong! Thanks for fixing it.
I agree; I tested it locally by just mapping a PLY file that's larger than my memory, but the exact behaviour of that depends on the overcommit settings of the machine, the amount of RAM, and Linux's heuristics if the |
Fixes plyfile incorrectly falling back to non-mmap read even
when mmap is supported.
Until now, for example, when the user wanted a read-only ("r") mmap
and that would succeed, plyfile's `_can_mmap()` check would not
use "r" but "c" for the check, which would fail for large files.
2 participants
Fixes plyfile incorrectly falling back to non-mmap read even when mmap is supported.
Until now, for example, when the user wanted a read-only ("r") mmap and that would succeed, plyfile's
_can_mmap()check would not use "r" but "c" for the check, which would fail for large files.That in turn would then cause OOMs, because with
_can_mmap() == Falseit would fall back to normal full reading.📚 Documentation preview 📚: https://python-plyfile--86.org.readthedocs.build/en/86/