-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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 |
@@ -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)) |
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.PyMiscIOTest
back to green. (A number have independent failures ontest_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)