GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-129005: Avoid copy in _pyio.FileIO.readinto by cmaloney · Pull Request #129006 · python/cpython

Conversation

@cmaloney

@cmaloney cmaloney commented Jan 18, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Reduce copies when reading files in pyio, match behavior of _io #129005

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cmaloney added 2 commits January 18, 2025 15:18
@cmaloney
pythongh-129005: Avoid copy in _pyio.FileIO.readinto
fa3dd18 
`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`
@cmaloney
add blurb
b4c985a
@bedevere-app bedevere-app Bot mentioned this pull request Jan 18, 2025
Reduce copies when reading files in pyio, match behavior of _io #129005
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jan 18, 2025
@tomasr8

tomasr8 commented Jan 18, 2025

Copy link
Copy Markdown
Member

os.readv is disabled on WASI (hence the failed test). We'll probably need to keep read as a fallback.
cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cmaloney added 3 commits January 18, 2025 15:52
@cmaloney
Add conditional code for platforms without os.readv
78643f0
@cmaloney
tweak for early exit
c309705
@cmaloney
Inline self.read, simplifying overall
8907ca2
tomasr8
tomasr8 approved these changes Jan 19, 2025
View reviewed changes

@tomasr8 tomasr8 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Misc/NEWS.d/next/Library/2025-01-18-15-29-08.gh-issue-129005.mM5dmV.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Jan 19, 2025
cmaloney and others added 3 commits January 19, 2025 09:19
@cmaloney @tomasr8
Update Misc/NEWS.d/next/Library/2025-01-18-15-29-08.gh-issue-129005.m…
a9ab9ee 
…M5dmV.rst

Co-authored-by: Tomas R. <[email protected]>
@cmaloney
Remove _pyio :meth: from NEWS as it results in a Sphinx warning
9e3ef78
@cmaloney
Merge branch 'main' into cmaloney/pyio_readinto
1646c6a
@cmaloney

cmaloney commented Jan 22, 2025

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

@tomasr8 tomasr8 tomasr8 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@cmaloney @tomasr8
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.