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

gh-129005: Avoid copy in _pyio.FileIO.readinto #129006

Closed
wants to merge 8 commits into from

Conversation

cmaloney
Copy link
Contributor

@cmaloney cmaloney commented Jan 18, 2025

os.read allocated and filled a buffer by calling read(2), than that data was copied into the user provied buffer. Read directly into the caller's buffer instead by using os.readv. self.read() was doing the closed and readable checks so move those into readinto

PR of readinto first as it is a lot smaller diff than the readall code but has the core os.read -> os.readv change. In main how the buffer is expanded between _io (new_buffersize) and _pyio (inline in read()) should get lined up.

`os.read` allocated and filled a buffer by calling `read(2)`, than that
data was copied into the user provied buffer. Read directly into the
caller's buffer instead by using `os.readv`. `self.read()` was doing the
closed and readable checks so move those into `readinto`
@tomasr8
Copy link
Member

tomasr8 commented Jan 18, 2025

os.readv is disabled on WASI (hence the failed test). We'll probably need to keep read as a fallback.

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

LGTM :)

@cmaloney
Copy link
Contributor Author

Thanks for the review @tomasr8 ! Going to close this PR while I work on #129205, then will make a new PR which uses that (See: #129005 (comment) for context if desired). Makes the same change simpler / no conditional around os.readv being supported on different platforms.

@cmaloney cmaloney closed this Jan 22, 2025
@cmaloney cmaloney deleted the cmaloney/pyio_readinto branch January 26, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants