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-120754: Refactor I/O modules to stash whole stat result rather than individual members by cmaloney · Pull Request #123412 · python/cpython

Conversation

@cmaloney

@cmaloney cmaloney commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

As I was working on gh-120754 I noticed I kept adding more members and copying out individual members from the fstat call, and that it may be simpler / easier to just stash (and invalidate) the whole stat result rather than individul members. This is preparatory work for

  1. Avoid calling isatty on open for regular files (Resolving Avoid calling isatty() for most open() calls #90102)
  2. Reduce system calls by making more members available (Helping implement Speed up open().read() pattern by reducing the number of system calls #120754)

One important note, and why the member is called stat_atopen is that the values should only be used as guidance / an estimate. With individual members copied out this was also the case. While it's common for a file to not be modified while python code is reading it, other processes could interact with it and code needs to handle that. Two examples of this that I've come across: It is possible to change an fd so isatty result changes (see: gh-90102 and GH-121941) and a fd which is opened in blocking mode may have an ioctl used on it to change it to non-blocking (see: gh-109523). The general class of bugs here are commonly called time-of-check to time of use (TOCTOU, https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use)

Given how common some specific patterns are (ex. Path().read_bytes()) it is still worthwhile to optimize those (Ex. disabling buffering results in a over 10% speedup in that case, GH-122111). The existing codepaths treated this correctly as far as I can tell.

This PR is a portion of GH-121593 which is being split up into smaller, hopefully easier to review chunks. Not calling isatty for regular files makes a small but measurable perf improvement for every "open and read whole regular file" python does.

Sorry, something went wrong.

Uh oh!

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

gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
cmaloney added 2 commits August 27, 2024 16:53
@cmaloney
pythongh-120754: Refactor _io to stash whole stat
508aa9d
Multiple places in the I/O stack optimize common cases by using the
information from stat. Currently individual members are extracted from
the stat and stored into the fileio struct. Refactor the code to store
the whole stat struct instead.
@cmaloney
pythongh-120754: Refactor _pyio to stash whole stat
9d849ce
Parallels the changes to _io. The `stat` Python object doesn't allow
changing members, so rather than modifying estimated_size, just clear
the value.
@bedevere-app bedevere-app Bot mentioned this pull request Aug 28, 2024
Speed up open().read() pattern by reducing the number of system calls #120754
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Aug 28, 2024
@cmaloney cmaloney mentioned this pull request Aug 28, 2024
GH-120754: Remove isatty call during regular open #121593
Closed
@cmaloney

cmaloney commented Aug 28, 2024

Copy link
Copy Markdown
Contributor Author

Could this get the no news tag? (This is changing / refactoring an implementation detail)

All reactions

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz added the skip news label Aug 28, 2024
vstinner
vstinner reviewed Aug 28, 2024
View reviewed changes
Comment thread Lib/_pyio.py Resolved

Uh oh!

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

Comment thread Modules/_io/fileio.c Resolved

Uh oh!

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

Comment thread Lib/_pyio.py Resolved

Uh oh!

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

Comment thread Modules/_io/fileio.c Resolved

Uh oh!

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

Comment thread Modules/_io/fileio.c Resolved

Uh oh!

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

Comment thread Lib/_pyio.py Resolved

Uh oh!

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

Comment thread Lib/_pyio.py Resolved

Uh oh!

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

cmaloney and others added 4 commits August 28, 2024 17:20
@cmaloney @vstinner
Apply suggestions from code review
bfcfcf2
Co-authored-by: Victor Stinner <[email protected]>
@cmaloney
Add comments around stat_atopen and why bufsize is + 1
3122665
@cmaloney
Apply review changes for _pyio _blkszie
8f5cfe4
@cmaloney
Fix comment formatting
d18a82d
vstinner
vstinner approved these changes Aug 29, 2024
View reviewed changes

@vstinner vstinner 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.

@gpshead @serhiy-storchaka @pitrou: Would you mind to have a look?

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/_pyio.py Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Aug 29, 2024
@cmaloney
Add _pyiio +1 comment to fileio for better clarity
c55d10e
@vstinner
vstinner merged commit 8b6c7c7 into python:main Sep 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 18, 2024
@vstinner

vstinner commented Sep 18, 2024

Copy link
Copy Markdown
Member

Ok, I merged your change. Thanks for your contribution. Let's see how it goes :-)

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
cmaloney deleted the cmaloney/stat_atopen branch September 18, 2024 19:04
@cmaloney

cmaloney commented Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

Looking at individual buildbots, seeing some test_io refleaks failures (https://buildbot.python.org/#/builders/259/builds/1384, https://buildbot.python.org/#/builders/551/builds/78), digging in a bit.

All reactions

Sorry, something went wrong.

Uh oh!

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

@zware zware mentioned this pull request Sep 18, 2024
Fix make htmllive target #124219
Merged
@vstinner

vstinner commented Sep 18, 2024

Copy link
Copy Markdown
Member

Using test.bisect_cmd, I identified the leaking test:

$ ./python -m test test_io -R 3:3 -m test.test_io.CIOTest.test_fileio_closefd
(...)
test_io leaked [1, 1, 1] memory blocks, sum=3
(...)
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.

@vstinner

vstinner commented Sep 18, 2024

Copy link
Copy Markdown
Member

Looking at individual buildbots, seeing some test_io refleaks failures (https://buildbot.python.org/#/builders/259/builds/1384, https://buildbot.python.org/#/builders/551/builds/78), digging in a bit.

I wrote a fix: PR gh-124225.

All reactions

Sorry, something went wrong.

Uh oh!

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

gpshead
gpshead reviewed Sep 18, 2024
View reviewed changes
Comment thread Modules/_io/fileio.c
get_blksize(fileio *self, void *closure)
{
#ifdef HAVE_STRUCT_STAT_ST_BLKSIZE
if (self->stat_atopen != NULL && self->stat_atopen->st_blksize > 1) {

@gpshead gpshead Sep 18, 2024

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.

I do wonder how realistic the st_blksize values, when available, are for performance purposes, I guess we'll find out.

Sorry, something went wrong.

Uh oh!

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

All reactions

@vstinner vstinner Sep 18, 2024

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.

This PR should not change the buffer size, does it?

Sorry, something went wrong.

Uh oh!

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

All reactions

@cmaloney cmaloney Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

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.

#117151 (comment) investigated st_blksize a bit previously. This PR I tried not to change buffer size at all / just change how it is accessed.

Have with the refactors + optimizations been watching for new issues. Are finding some as people test main (ex. gh-113977 which I wrote a primary fix for #122101, and have more fix ideas on top of the stat_atopen changes)

Sorry, something went wrong.

Uh oh!

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

gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@cmaloney @savannahostrowski
pythongh-120754: Refactor I/O modules to stash whole stat result rath…
b97ec9c
…er than individual members (python#123412)

Multiple places in the I/O stack optimize common cases by using the
information from stat. Currently individual members are extracted from
the stat and stored into the fileio struct. Refactor the code to store
the whole stat struct instead.

Parallels the changes to _io. The `stat` Python object doesn't allow
changing members, so rather than modifying estimated_size, just clear
the value.
This was referenced Oct 3, 2024
gh-90102: Remove isatty call during regular open #124922
Merged
gh-117151: IO performance improvement, increase io.DEFAULT_BUFFER_SIZE to 128k #118144
Merged
@scasella scasella mentioned this pull request Sep 26, 2026
Free-threading: concurrent FileIO close/truncate can double-free stat_atopen #158196
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@vstinner vstinner vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

4 participants

@cmaloney @vstinner @gpshead @picnixz
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.