Skip to content

gh-129005: Fix buffer expansion in _pyio.FileIO.readall #129541

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

Closed
wants to merge 1 commit into 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).

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 2, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 755b385 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F30617%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 2, 2025
@cmaloney
Copy link
Contributor Author

cmaloney commented Feb 2, 2025

will open a new PR on top of the reverts + gh-129559

@cmaloney cmaloney closed this Feb 2, 2025
@cmaloney cmaloney deleted the fix_blocking_test_failures branch February 2, 2025 19:01
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.

4 participants