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

BufferedIOBase.readinto1 is missing #64777

Closed
nikratio mannequin opened this issue Feb 10, 2014 · 22 comments
Closed

BufferedIOBase.readinto1 is missing #64777

nikratio mannequin opened this issue Feb 10, 2014 · 22 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@nikratio
Copy link
Mannequin

nikratio mannequin commented Feb 10, 2014

BPO 20578
Nosy @loewis, @pitrou, @vstinner, @benjaminp, @hynek
Files
  • issue20578.diff
  • issue20578_r2.diff
  • benchmark.py
  • benchmark.py
  • issue20578_r3.diff
  • benchmark_r3.py
  • issue20578_r4.diff
  • issue20578_r5.diff
  • issue20578_r6.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2014-06-22.21:17:55.992>
    created_at = <Date 2014-02-10.00:13:49.017>
    labels = ['type-feature', 'library']
    title = 'BufferedIOBase.readinto1 is missing'
    updated_at = <Date 2014-06-22.21:17:55.991>
    user = 'https://bugs.python.org/nikratio'

    bugs.python.org fields:

    activity = <Date 2014-06-22.21:17:55.991>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-06-22.21:17:55.992>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2014-02-10.00:13:49.017>
    creator = 'nikratio'
    dependencies = []
    files = ['34632', '34793', '34811', '34812', '34863', '34864', '35539', '35647', '35648']
    hgrepos = []
    issue_num = 20578
    keywords = ['patch']
    message_count = 22.0
    messages = ['210794', '210795', '214930', '215263', '215979', '215984', '215988', '215989', '216049', '216053', '216055', '216056', '216059', '216266', '220012', '220015', '220018', '220061', '220062', '220662', '220665', '221313']
    nosy_count = 8.0
    nosy_names = ['loewis', 'pitrou', 'vstinner', 'benjamin.peterson', 'stutzbach', 'nikratio', 'python-dev', 'hynek']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20578'
    versions = ['Python 3.5']

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Feb 10, 2014

    It would be nice to have a readinto1 method to complement the existing read, readinto, and read1 methods of io.BufferedIOBase.

    @nikratio nikratio mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 10, 2014
    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Feb 10, 2014

    (I'll work on a patch for this myself, this bug is just to prevent duplicate work)

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Mar 27, 2014

    I have attached a patch that adds readinto1() to BufferedReader and BufferedRWPair.

    An example use case for this method is receiving a large stream over a protocol like HTTP. You want to use a buffered reader so you can efficiently parse the header, but after that you want to stream the data as it comes in, i.e. you want to use read1 or, for improved performance, readinto1.

    Feedback is welcome.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 31, 2014

    The IO APIs are a visible part of the stdlib, so I'd suggest asking on the python-dev mailing-list first.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 12, 2014

    Seems as if no one has an opinion on this at all: https://mail.python.org/pipermail/python-dev/2014-April/133739.html

    What next?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 12, 2014

    I put some review into rietveld. In addition, please avoid code duplication of more than three subsequent lines of code.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 12, 2014

    Thanks for the review! Attached is a new patch. I was actually pretty careful to avoid any code duplication.. are you refering to the readinto1() implementations for BytesIO and BufferedReader in Lib/_pyio.py?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 13, 2014

    Put up a new review.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 14, 2014

    Here's a little script to estimate the performance difference between using read1 and readinto1 to read large amounts of data. On my system, I get:

    C readinto1: 4.960e-01 seconds
    C read1: 4.055e-01 seconds
    Python readinto1: 1.066e+00 seconds
    Python read1: 2.565e+00 seconds

    In other words, _pyio.BufferedReader.readinto1 is more than a factor of 2 faster than _pyio.BufferedReader.read1 and io.readinto1 is faster than io.read1 by about 20%.

    On its own, I think this would justify keeping an implementation of readinto1 in _pyio.BufferedReader instead of falling back on the default (that is implemented using read1). However, I believe that people who need performance are probably not using _pyio but io, so *my* argument for keeping it implemented in _pyio is to keep the implementations similiar.

    I found studying _pyio very helpful to understand the C code in io. If we implement BufferedReader.readinto1 in io, but not in _pyio.BufferedReader, this advantage would be reduced.

    That said, I am primary interested in getting readinto1 into io. So I'm happy to either extend the patch to also provide a fast readinto implementation for _pyio (to align it with io), or to remove the readinto1 implementation in _pyio.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 14, 2014

    Can you please extend your benchmark to also measure read and readinto?

    I'm puzzled why you are treating readinto1 differently from readinto.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 14, 2014

    (Rietveld is giving me errors, so I'm replying here)

    On 2014/04/13 02:22:23, loewis wrote:
    >>> Again, why a separate implementation here?
    >> 
    >> For performance reasons. Relying on the default implementation
    >> would fall back to using read1(), which means a new bytes object
    >> is created first.
    > 
    > Hmm.
    > a) if performance was relevant, it should apply to readinto() as well.

    I didn't even notice the readinto implementation was missing. But I
    agree, if we keep readinto1(), we should also add readinto().

    b) if the code is duplicated for performance only, a comment should
    state that explicitly.

    I'm very sorry, but I still don't see which code in readinto1() is
    duplicate. You don't mean duplicate between io and _pyio, do you?

    c) to justify a performance argument, you should really provide hard
    numbers that demonstrate a performance gain justifying the code
    duplication.

    I posted a small benchmark to the issue tracker. Personally, I think
    the more important argument is to keep the Python and C
    implementations similar. It's really nice if you can look at _pyio to
    find out at least roughly what happens in io.

    (Yes, I did put performance first in my last reply, but only because I
    thought you were asking about readinto1 in general, rather than the
    additional Python implementation in _pyio.)

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 14, 2014

    Can you please extend your benchmark to also measure read and readinto?

    Yes - but I don't quite understand why it matters (if you need read1/readinto1, you cannot just use read/readinto instead).

    C readinto1: 4.638e-01 seconds
    C read1: 4.026e-01 seconds
    C readinto: 4.655e-01 seconds
    C read: 4.028e-01 seconds
    Python readinto1: 1.056e+00 seconds
    Python read1: 2.429e+00 seconds
    Python readinto: 1.895e+00 seconds
    Python read: 1.218e+00 seconds

    That shows that the Python readinto is definetely not up-to-par and could use improvement as well. Is that what you're getting at?

    I'm puzzled why you are treating readinto1 differently from readinto.

    Maybe this is why we seem to be talking past each other :-). I did not look or work on readinto at all. All I noticed is that there is a read1, but no readinto1. So I implemented a readinto1 as well as I could.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 14, 2014

    I didn't even notice the readinto implementation was missing. But I
    agree, if we keep readinto1(), we should also add readinto().
    [...]
    Maybe this is why we seem to be talking past each other :-). I did not
    look or work on readinto at all. All I noticed is that there is a read1,
    but no readinto1. So I implemented a readinto1 as well as I could.

    I see. It's not actually true that there is no readinto - it's inherited from the base class.

    I think it is more important that the implementation is consistent than that it is performant (but achieving both should be possible).

    Whether or not _pyio needs to be performant, I don't know. Having it consistent with _io would be desirable, but might not be possible.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Apr 15, 2014

    Attached is an updated patch that

    • removes the code duplication in _pyio.BufferedIOBase
    • adds an internal _readinto helper method to _pyio.BufferedReader that makes the implementation similar to io.BufferedReader.
    • implements _pyio.BuffereadReader.{readinto,readinto1} in terms of the new helper method and, as a side effect, also increases their performance.

    Performance of the _pyio implementation on my system is:

    pre-patch:
    readinto: 5.130e+00 seconds
    readinto1 not available

    post-patch:
    readinto: 2.039e+00 seconds
    readinto1: 1.995e+00 seconds

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2014

    New changeset 0fb7789b5eeb by Benjamin Peterson in branch 'default':
    add BufferedIOBase.readinto1 (closes bpo-20578)
    http://hg.python.org/cpython/rev/0fb7789b5eeb

    @python-dev python-dev mannequin closed this as completed Jun 8, 2014
    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jun 8, 2014

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2014

    New changeset b1e99b4ec374 by Benjamin Peterson in branch 'default':
    backout 0fb7789b5eeb for test breakage (bpo-20578)
    http://hg.python.org/cpython/rev/b1e99b4ec374

    @benjaminp benjaminp reopened this Jun 8, 2014
    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Jun 8, 2014

    Thanks for taking the time, and apologies about the test failure. I was probably too eager and ran only the test_io suite instead of everything.

    I looked at the failure, and the problem is that the default Python BufferedIOBase.readinto implementation is semi-broken. It should work with any object implementing the memoryview protocol (like the C implementation), but it really only works with bytearray objects. The testIteration test only worked (prior to the patch) because there is a special case for array objects with format 'b':

        try:
            b[:n] = data
        except TypeError as err:
            import array
            if not isinstance(b, array.array):
                raise err
            b[:n] = array.array('b', data)
    

    In other words, trying to read into any other object has always failed. In particular, even format code 'B' fails:

    >>> import _pyio
    >>> from array import array
    >>> buf = array('b', b'x' * 10)
    >>> _pyio.open('/dev/zero', 'rb').readinto(buf) 
    10
    >>> buf = array('B', b'x' * 10)
    >>> _pyio.open('/dev/zero', 'rb').readinto(buf)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/nikratio/clones/cpython/Lib/_pyio.py", line 1096, in readinto
        buf[:len_] = array.array('b', buf2)
    TypeError: bad argument type for built-in operation

    The readline implementation that my patch adds for BufferedReader does not contain this special case, and therefore with the patch even the test with a 'b'-array fails.

    For now, I've added the same special casing of 'b'-type arrays to the _readline() implementation in BufferedReader. This fixes the immediate problem (and this time I definitely ran the entire testsuite).

    However, the fix is certainly not what I would consider a good solution.. but I guess that would better be addressed by a separate patch that also fixes the same issue in BufferedIOBase?

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Jun 8, 2014

    I used the wrong interpreter when cutting and pasting the example above, here's the correct version to avoid confusion with the traceback:

    >>> import _pyio
    >>> from array import array
    >>> buf = array('b', b'x' * 10)
    >>> _pyio.open('/dev/zero', 'rb').readinto(buf) 
    10
    >>> buf = array('B', b'x' * 10)
    >>> _pyio.open('/dev/zero', 'rb').readinto(buf)
    Traceback (most recent call last):
      File "/home/nikratio/clones/cpython/Lib/_pyio.py", line 662, in readinto
        b[:n] = data
    TypeError: can only assign array (not "bytes") to array slice
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/nikratio/clones/cpython/Lib/_pyio.py", line 667, in readinto
        b[:n] = array.array('b', data)
    TypeError: bad argument type for built-in operation

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Jun 15, 2014

    As discussed on python-devel, I'm attaching a new patch that uses memoryview.cast to ensure that the pure-Python readinto() now works with any object implementing the buffer protocol.

    @nikratio
    Copy link
    Mannequin Author

    nikratio mannequin commented Jun 15, 2014

    (refreshed patch, no changes)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 22, 2014

    New changeset 213490268be4 by Benjamin Peterson in branch 'default':
    add BufferedIOBase.readinto1 (closes bpo-20578)
    http://hg.python.org/cpython/rev/213490268be4

    @python-dev python-dev mannequin closed this as completed Jun 22, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants