Add os.readinto API for reading data into a caller provided buffer

[cmaloney]

Feature or enhancement

Proposal:

Code reading data in pure python tends to make a buffer variable, call os.read() which returns a separate newly allocated buffer of data, then copy/append that data onto the pre-allocated buffer[0]. That creates unnecessary extra buffer objects, as well as unnecessary copies. Provide os.readinto for directly filling a Buffer Protocol object.

os.readinto should closely mirror _Py_read which underlies os.read in order to get the same behaviors around retries as well as well-tested cross-platform support.

Move simple cases that use os.read (ex. [0]) to use the new API when it makes code simpler and more efficient. Potentially adding readinto to more readable/writeable file-like proxy objects or objects which transform the data (ex. Lib/_compression) is out of scope for this issue.

[0]

cpython/Lib/subprocess.py

Lines 1914 to 1921 in 298dda5

	# Wait for exec to fail or succeed; possibly raising an
	# exception (limited in size)
	errpipe_data = bytearray()
	while True:
	part = os.read(errpipe_read, 50000)
	errpipe_data += part
	if not part or len(errpipe_data) > 50000:
	break

cpython/Lib/multiprocessing/forkserver.py

Lines 384 to 392 in 298dda5

	def read_signed(fd):
	data = b''
	length = SIGNED_STRUCT.size
	while len(data) < length:
	s = os.read(fd, length - len(data))
	if not s:
	raise EOFError('unexpected EOF')
	data += s
	return SIGNED_STRUCT.unpack(data)[0]

cpython/Lib/_pyio.py

Lines 1695 to 1701 in 298dda5

	def readinto(self, b):
	"""Same as RawIOBase.readinto()."""
	m = memoryview(b).cast('B')
	data = self.read(len(m))
	n = len(data)
	m[:n] = data
	return n

os.read loops to migrate

Well contained os.read loops

• multiprocessing.forkserver read_signed - @cmaloney - gh-129205: Update multiprocessing.forkserver to use os.readinto #129425
• [x] subprocess Popen._execute_child - @cmaloney - gh-129205: Use os.readinto() in subprocess errpipe_read #129498

os.read loop interleaved with other code

• _pyio FileIO.read FileIO.readall FileIO.readinto see, Reduce copies when reading files in pyio, match behavior of _io #129005 -- @cmaloney
• _pyrepl.unix_console UnixConsole.input_buffer -- fixed length underlying buffer with "pos" / window on top.
• pty _copy. Operates around a "high waterlevel" / attempt to have a fixed-ish size buffer. Wraps os.read with a _read function.
• subprocess Popen.communicate. Note, this feels like something non-contiguous Py_buffer would be really good for, particularly in self.text_mode where currently all the bytes are "copied" into a contiguous bytes to turn then turn into text...
• tarfile _Stream._read and _Stream.__read. Note, builds _LowLevelFile around os.read, but other read methods also available.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

#129005 (comment)

Linked PRs

• gh-129205: Add os.readinto API for reading data into a caller provided buffer #129211
• gh-129205: Modernize test_eintr #129316
• gh-129205: Update multiprocessing.forkserver to use os.readinto #129425
• gh-129205: Use os.readinto() in subprocess errpipe_read #129498

[vstinner]

Do you want to work on a PR? If not, I can if you prefer.

[cmaloney]

Almost done with one for adding os.readinto + tests :). Just trying to make sure I port / replicate the right os.read tests (but not all / _Py_read is used by both and handles most cases). Definitely be nice to both work + review os.read -> os.readinto as appropriate if you're up for that (there's quite a bit in multiprocessing, subprocess, ...).

[bluetech]

Just curious, how would you rewrite your first example using os.readinto?

[cmaloney]

@bluetech Ideally to me they'd move to file.read() / the io file object stack which would give the desired "read until EOF or max_bytes is reached efficiently" behavior. Already that is efficient in a lot of cases, but can't be used everywhere currently. Mixture of some overhead that would need to be reduced to match performance (locks, buffer allocation, etc) + relative age of different parts of the codebase (io came in Python 3.0, many other python modules are older). That case I would like to be a BufferedIO with a 0-byte buffer doing a .read() with a "max_size" parameter, which needs a little bit of new features on BufferedIO to handle the use case. Currently though it's open coded.

Migrating cases like that should happen, and I suspect whats the simplest/cleanest will evolve with code review. My prototype has looked something like:

errpipe_data = bytearray(50_000)
bytes_read = 0
while bytes_read < 50_000:
    count := os.readinto(errpipe_read, memoryview(errpipe_data)[bytes_read:]):
    if count == 0:
        break
    bytes_read += count
del errpipe_data[bytes_read:]  # Remove excess bytes

Are some behavior differences between that and the code as implemented today (Today after reading 49_999 bytes, could get 50_000 bytes resulting in 99_999 bytes in errpipe_data, but my prototype never goes past 50_000 bytes).

Not sure if it's good / needed to include the -1 case (and probably something I should add a test around.. Returning 0, number of bytes read, or throwing an exception would definitely simplify call site hadnling a bit...

I also like doing the same thing but with a match instead of the if.

[bluetech]

Are some behavior differences between that and the code as implemented today (Today after reading 49_999 bytes, could get 50_000 bytes resulting in 99_999 bytes in errpipe_data, but my prototype never goes past 50_000 bytes).

Side note, it does look like the original code is meant to be len(errpipe_data) >= 50000 instead of >, but it doesn't matter much I suppose.

[cmaloney]

os.read loops to migrate

I added a list to the base issue of os.read loops I think could be migrated. Comment if there's one you want to claim, and/or just cc: @cmaloney me in the PR (I don't have access to the sub task lists preview which the audit thread safety issue uses unfortunately)

The interleaved with other code/classes may be not be worth migrating / difficult to refactor without breaking existing code.

There are a number of places which do "IO like" objects with .read methods, excluding those from this issue. Some cases are likely worth investigating independently, but there is a lot more variation and surrounding context needed to make changes.

I think there's also some significant code golf around "shortest way to use os.readinto", my current variant has an issue of one more readinto call than needed in the "got all bytes the first call case" (which is the common fast path...).

[gpshead]

The subprocess _execute_child one doesn't need changing, see rationale in the closed PR.

readinto an existing bytearray has a caveat: instead of a malloc, it does a malloc + bzero as pre-sized bytearrays don't have an undefined value concept. so it goes beyond just mapping pages for a buffer to writing all pages, no matter how much data is actually being read. when you're rarely/never going to be reading a lot of data or reusing the buffer, that is unneeded extra work.

[cmaloney]

@gpshead I commented on the closed PR, neither bytearray nor bytes in the code I've read does a zeroing of its initial allocation. Just does a malloc, with bytearray adding the one caveat of writing a single null-byte at the end of the array.

Will definitely take as a general note not to migrate loops unless there's a measurable / significant performance change.