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

Jump to bottom

Why does http.client.HTTPResponse._safe_read use MAXAMOUNT #80231

Closed
bmerry mannequin opened this issue Feb 20, 2019 · 11 comments
Closed

Why does http.client.HTTPResponse._safe_read use MAXAMOUNT #80231

bmerry mannequin opened this issue Feb 20, 2019 · 11 comments
Labels
3.8 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@bmerry
Copy link
Mannequin

bmerry mannequin commented Feb 20, 2019

BPO 36050
Nosy @methane, @ambv, @vadmium, @bmerry
PRs
  • bpo-36050: optimize HTTPResponse.read() #12698
    		•

    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 2021-07-29.15:57:11.886>
created_at = <Date 2019-02-20.12:55:43.858>
labels = ['3.8', 'library', 'performance']
title = 'Why does http.client.HTTPResponse._safe_read use MAXAMOUNT'
updated_at = <Date 2021-07-29.16:22:53.071>
user = 'https://github.com/bmerry'

    bugs.python.org fields:

    activity = <Date 2021-07-29.16:22:53.071>
actor = 'bmerry'
assignee = 'none'
closed = True
closed_date = <Date 2021-07-29.15:57:11.886>
closer = 'lukasz.langa'
components = ['Library (Lib)']
creation = <Date 2019-02-20.12:55:43.858>
creator = 'bmerry'
dependencies = []
files = []
hgrepos = []
issue_num = 36050
keywords = ['patch']
message_count = 11.0
messages = ['336081', '336498', '339496', '339498', '339532', '398376', '398387', '398389', '398493', '398495', '398496']
nosy_count = 4.0
nosy_names = ['methane', 'lukasz.langa', 'martin.panter', 'bmerry']
pr_nums = ['12698']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue36050'
versions = ['Python 3.8']
    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Feb 20, 2019

    While investigating poor HTTP read performance I discovered that reading all the data from a response with a content-length goes via _safe_read, which in turn reads in chunks of at most MAXAMOUNT (1MB) before stitching them together with b"".join. This can really hurt performance for responses larger than MAXAMOUNT, because
    (a) the data has to be copied an additional time; and
    (b) the join operation doesn't drop the GIL, so this limits multi-threaded scaling.

    I'm struggling to see any advantage in doing this chunking - it's not saving memory either (in fact it is wasting it).

    To give an idea of the performance impact, changing MAXAMOUNT to a very large value made a multithreaded test of mine go from 800MB/s to 2.5GB/s (which is limited by the network speed).

    @bmerry bmerry mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Feb 20, 2019
    @vadmium
    Copy link
    Member

    vadmium commented Feb 25, 2019

    The 1 MiB limit was added for bpo-1296004; apparently some platforms were overallocating multiple buffers and running out of memory. I suspect the loop in "_safe_read" was inherited from Python 2, which has different kinds of file objects. The comments suggest it does partial reads.

    But the Python 3 code calls "socket.makefile" with "buffering" mode enabled by default. I agree it should be safe to read the total length in one go.

    @methane
    Copy link
    Member

    methane commented Apr 5, 2019

    bpo-1296004 is too old (512MB RAM machine!) and I can not confirm it.

    But I think it was caused by inefficient realloc() in CRT.

    See

    cpython/Lib/socket.py

    Lines 298 to 303 in 6c52d76

    while True:
    left = size - buf_len
    recv_size = max(self._rbufsize, left)
    data = self._sock.recv(recv_size)
    if not data:
    break

    _fileobject called socket.recv with remaining size.
    Typically, socket can't return MBs at once. So it cause:

    1. Large (at most amt, some MBs) string (bytes) are allocated. (malloc)
    2. recv is called.
    3. _PyString_Resize() (realloc) is called with smaller bytes (typically ~128KiB)
    4. amt -= received
    5. if amt == 0: exit; goto 1.

    This might stress malloc and realloc in CRT. It caused fragmentation and MemoryError.

    ---

    For now, almost everything is rewritten.

    In case of _pyio, BufferedIOReader calls RawIOBase.read(). It copies from bytearray to bytes. So call only malloc and free. Stress for realloc will be reduced.

    cpython/Lib/_pyio.py

    Lines 586 to 591 in 50866e9

    b = bytearray(size.__index__())
    n = self.readinto(b)
    if n is None:
    return None
    del b[n:]
    return bytes(b)

    In case of C _io module, it is more efficient. It allocate PyBytes once and calls SocketIO.read_into directly. No temporary bytes objects are created.

    cpython/Modules/_io/bufferedio.c

    Line 1632 in 50866e9

    res = PyBytes_FromStringAndSize(NULL, n);

    cpython/Modules/_io/bufferedio.c

    Line 1658 in 50866e9

    r = _bufferedreader_raw_read(self, out + written, r);

    cpython/Modules/_io/bufferedio.c

    Line 1470 in 50866e9

    res = PyObject_CallMethodObjArgs(self->raw, _PyIO_str_readinto, memobj, NULL);

    @methane methane added 3.8 only security fixes performance Performance or resource usage and removed 3.7 (EOL) end of life labels Apr 5, 2019
    @methane
    Copy link
    Member

    methane commented Apr 5, 2019

    Additionally, _safe_read calls fp.read() multiple times to handle EINTR.
    But EINTR is handled by socket module now (PEP-475).

    Now the function can be very simple.

    @methane
    Copy link
    Member

    methane commented Apr 6, 2019

    New changeset d6bf6f2 by Inada Naoki in branch 'master':
    bpo-36050: optimize HTTPResponse.read() (GH-12698)
    d6bf6f2

    @methane methane closed this as completed Apr 6, 2019
    @ambv
    Copy link
    Contributor

    ambv commented Jul 28, 2021

    New changeset 153365d by Inada Naoki in branch '3.9':
    [3.9] bpo-42853: Fix http.client fails to download >2GiB data over TLS (GH-27405)
    153365d

    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Jul 28, 2021

    Re-opening because the patch to fix this has just been reverted due to bpo-42853.

    @bmerry bmerry mannequin reopened this Jul 28, 2021
    @methane
    Copy link
    Member

    methane commented Jul 28, 2021

    Note that it was reverted only in 3.9 branch.

    @ambv
    Copy link
    Contributor

    ambv commented Jul 29, 2021

    Inadasan is right, this is still fixed in 3.10 and 3.11. The 3.9 revert is due to 3.9 supporting OpenSSL older than 1.1.1. 3.10+ requires OpenSSL 1.1.1+ per PEP-644.

    There is nothing to do here.

    @ambv ambv closed this as completed Jul 29, 2021
    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Jul 29, 2021

    There is nothing to do here.

    Will you accept patches to fix this for 3.9? I'm not clear whether the "bug fixes only" status of 3.9 allows for fixing performance regressions.

    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Jul 29, 2021

    Will you accept patches to fix this for 3.9? I'm not clear whether the "bug fixes only" status of 3.9 allows for fixing performance regressions.

    Never mind, I see your already answered this on bpo-42853 (as a no). Thanks for taking the time to answer my questions; I'll just have to skip Python 3.9 for this particular application and go straight to 3.10.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @illia-v illia-v mentioned this issue Jun 10, 2022
    Closed
    3 tasks
    @illia-v illia-v mentioned this issue Jul 3, 2022
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.8 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @ambv @methane @vadmium