GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-129005: Align FileIO.readall between _pyio and _io by cmaloney · Pull Request #129705 · python/cpython Conversation
|
| result += chunk | ||
|
|
||
| bytes_read += n | ||
| result.resize(bytes_read) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
result = memoryview(result)[bytes_read:] would avoid a truncation which can imply a memory copy in the worst case, no?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
the resize "shrink" in bytearray doesn't actually resize unless the buffer's "capacity" is 2x the requested size (https://github.com/python/cpython/blob/main/Objects/bytearrayobject.c#L201-L214). Just updates its internal "this is how long the bytes is" counter (which for things like full-file readall with known size, this should already be just one byte over the right size).
My plan currently is to make it so bytes(bytearray(10)) and bytearray(b'\0' * 10) both don't copy (Ongoing discussion in https://discuss.python.org/t/add-zero-copy-conversion-of-bytearray-to-bytes-by-providing-bytes/79164). Having a memoryview would mean there's more than one reference to the bytearray, and I couldn't do / use that optimization.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm fine with using result.resize() here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <[email protected]>
|
Hypothesis test failure in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| bufsize += max(bufsize, DEFAULT_BUFFER_SIZE) | ||
| n = bufsize - len(result) | ||
| if bytes_read >= bufsize: | ||
| # Parallels _io/fileio.c new_buffersize |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
In the C code, new_buffersize() argument is bytes_read, not bufsize. You may keep new_buffersize() as a private module-level function.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Updated the loop to no longer use bufsize at all, this is the only line that used it, and it feels more Pythonic to me to just use len(result).
That enables rewriting to:
try: # Read until EOF (n == 0) while n := os.readinto(self._fd, memoryview(result)[bytes_read:]): bytes_read += n if bytes_read >= len(result): result.resize(_new_buffersize(bytes_read)) except BlockingIOError: if not bytes_read: return None assert len(result) - bytes_read >= 1, \ "os.readinto buffer size 0 will result in erroneous EOF / returns 0" result.resize(bytes_read) return bytes(result)
which feels cleaner, but also starts changing structure relative to _io version.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
decided to refactor to this. Control flow feels a lot simpler to me and a lot more readable than the branches and breaks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Tests / Windows / build and test (Win32) failure is a |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merged, thank you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.