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: Fix buffer expansion in _pyio.FileIO.readall #129541

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cmaloney
Copy link
Contributor

@cmaloney cmaloney commented Feb 1, 2025

Move to a linear slice append with an iterator which has a length hint. This is more expensive then PyByteArray_Resize, but I think as efficient as can get without a new bytearray Python API to resize.

The previous code didn't append as I had intended:

a = bytearray()
>>> a[0:5] = b'\0'
>>> a
bytearray(b'\x00')
>>> a[5:16] = b'\01'
>>> a
bytearray(b'\x00\x01')
>>> len(a)
2

This should get the buildbots which are failing on test_io.test_io.PyMiscIOTest back to green. (A number have independent failures on test_getpath still)

What bytearray.resize looks like (Wrapping the C API): https://github.com/python/cpython/compare/main...cmaloney:cpython:resize_fixup?expand=0 (Currently that returns an int, but probably would make sense to return None)

Move to a linear slice append with an iterator which has a length hint.
This is more expensive then PyByteArray_Resize, but I think as efficient
as can get without a new bytearray Python API to resize.

The previous code didn't append as I had intended:

```python
a = bytearray()
>>> a[0:5] = b'\0'
>>> a
bytearray(b'\x00')
>>> a[5:16] = b'\01'
>>> a
bytearray(b'\x00\x01')
>>> len(a)
2
```
@cmaloney
Copy link
Contributor Author

cmaloney commented Feb 1, 2025

@vstinner this should get remaining bots failing on test_io.test_io.PyMiscIOTest back to green. This patch fixes the underlying issue: readall didn't actually readall, as the buffer isn't as big as the code thought, it slices it to bytes_read resulting in a 0-size buffer being passed to osreadinto, which returns 0, which ends the readall.

@@ -1686,7 +1687,8 @@ def readall(self):
if addend < DEFAULT_BUFFER_SIZE:
addend = DEFAULT_BUFFER_SIZE
bufsize += addend
result[bytes_read:bufsize] = b'\0'
result.extend(repeat(0, addend))
Copy link
Member

Choose a reason for hiding this comment

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

Why not just b'\0' * addend?

Copy link
Contributor Author

@cmaloney cmaloney Feb 1, 2025

Choose a reason for hiding this comment

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

That uses a lot more memory (And overall goal is to get _io and _pyio to use the same). result is expanded in length by addend bytes, a new bytes is constructed which is of length addend containing just null, then there's a memcpy from the new temporary bytes object to result... That means get 2x addend memory usage, rather than just expanding the buffer by addend bytes.

In this case, bytearray.extend also doesn't prefer working in place / creates a PyByteArray_FromStringAndSize that it copies the data out of a iterator on the object passed to it as an argument...

My preference is definitely bytearray.resize which does what I'd need here, but didn't want that to be a blocker for getting bots back to green (#129560). Reading through the slice, extend, append, etc. code in bytearray haven't found any other efficient ways to just "expand capacity, in place if possible without multiplying existing space" and (ideally) without requiring a by-byte write to every byte (the read loop is about to do that anyways).

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