-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
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
Conversation
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 ```
|
@vstinner this should get remaining bots failing on |
| addend = DEFAULT_BUFFER_SIZE | ||
| bufsize += addend | ||
| result[bytes_read:bufsize] = b'\0' | ||
| result.extend(repeat(0, addend)) |
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.
Why not just b'\0' * addend?
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.
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).
|
🤖 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. |
|
will open a new PR on top of the reverts + gh-129559 |
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:
This should get the buildbots which are failing on
test_io.test_io.PyMiscIOTestback to green. (A number have independent failures ontest_getpathstill)What
bytearray.resizelooks 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)