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

HTTPResponse.read with amt is slow #85174

Closed
bmerry mannequin opened this issue Jun 17, 2020 · 5 comments
Closed

HTTPResponse.read with amt is slow #85174

bmerry mannequin opened this issue Jun 17, 2020 · 5 comments
Labels
3.10 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@bmerry
Copy link
Mannequin

bmerry mannequin commented Jun 17, 2020

BPO 41002
Nosy @methane, @miss-islington, @bmerry, @openandclose
PRs
  • bpo-41002: Optimize HTTPResponse.read with a given amount #20943
  • Files
  • httpbench-simple.py: Micro-benchmark to demonstrate the issue
  • 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 2020-06-25.06:31:52.469>
    created_at = <Date 2020-06-17.12:08:15.951>
    labels = ['library', '3.10', 'performance']
    title = 'HTTPResponse.read with amt is slow'
    updated_at = <Date 2020-06-25.06:31:52.469>
    user = 'https://github.com/bmerry'

    bugs.python.org fields:

    activity = <Date 2020-06-25.06:31:52.469>
    actor = 'methane'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-06-25.06:31:52.469>
    closer = 'methane'
    components = ['Library (Lib)']
    creation = <Date 2020-06-17.12:08:15.951>
    creator = 'bmerry'
    dependencies = []
    files = ['49239']
    hgrepos = []
    issue_num = 41002
    keywords = ['patch']
    message_count = 5.0
    messages = ['371732', '371821', '371822', '371824', '372304']
    nosy_count = 4.0
    nosy_names = ['methane', 'miss-islington', 'bmerry', 'op368']
    pr_nums = ['20943']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue41002'
    versions = ['Python 3.10']

    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Jun 17, 2020

    I've run into this on 3.8, but the code on Git master doesn't look significantly different so I assume it still applies. I'm happy to work on a PR for this.

    When http.client.HTTPResponse.read is called with a specific amount to read, it goes down this code path:

    if amt is not None:
        # Amount is given, implement using readinto
        b = bytearray(amt)
        n = self.readinto(b)
        return memoryview(b)[:n].tobytes()
    

    That's pretty inefficient, because

    • bytearray(amt) will first zero-fill some memory
    • tobytes() will make an extra copy of this memory
    • if amt is big enough, it'll cause the temporary memory to be allocated from the kernel, which will also zero-fill the pages for security.

    A better approach would be to use the read method of the underlying fp.

    I have a micro-benchmark (that I'll attach) showing that for a 1GB body and reading the whole body with or without the amount being explicit, performance is reduced from 3GB/s to 1GB/s.

    For some unknown reason the requests library likes to read the body in 10KB chunks even if the user has requested the entire body, so this will help here (although the gains probably won't be as big because 10KB is really too small to amortise all the accounting overhead).

    Output from my benchmark, run against a 1GB file on localhost:

    httpclient-read: 3019.0 ± 63.8 MB/s
    httpclient-read-length: 1050.3 ± 4.8 MB/s
    httpclient-read-raw: 3150.3 ± 5.3 MB/s
    socket-read: 3134.4 ± 7.9 MB/s

    @bmerry bmerry mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir performance Performance or resource usage labels Jun 17, 2020
    @methane methane added 3.10 only security fixes and removed 3.8 only security fixes labels Jun 18, 2020
    @openandclose
    Copy link
    Mannequin

    openandclose mannequin commented Jun 18, 2020

    @bmerry check the test results again.
    (perhaps 'MB/s's are wrong).

    httpclient-read: 3019.0 ± 63.8 MB/s
    httpclient-read-length: 1050.3 ± 4.8 MB/s
    

    --> httpclient-read-raw: 3150.3 ± 5.3 MB/s
    --> socket-read: 3134.4 ± 7.9 MB/s

    @bmerry
    Copy link
    Mannequin Author

    bmerry mannequin commented Jun 18, 2020

    (perhaps 'MB/s's are wrong).

    Why, are you getting significantly different results?

    Just in case it's confusing, the results are reported as A ± B MB/s, where A is the mean and B is the standard deviation of the mean. So it's about 3GB/s when no length if passed, or 1GB/s when a length is passed.

    @openandclose
    Copy link
    Mannequin

    openandclose mannequin commented Jun 18, 2020

    @bmerry yah, sorry, don't bother. I have mistaken.
    (I thought somehow 'MB/s' was simple speed, not std).

    I confirmed your tests.
    httpclient-read: 2504.5 ± 10.6 MB/s
    httpclient-read-length: 871.5 ± 4.9 MB/s
    httpclient-read-raw: 2528.3 ± 3.6 MB/s
    socket-read: 2520.9 ± 3.6 MB/s

    @miss-islington
    Copy link
    Contributor

    New changeset 152f0b8 by Bruce Merry in branch 'master':
    bpo-41002: Optimize HTTPResponse.read with a given amount (GH-20943)
    152f0b8

    @methane methane closed this as completed Jun 25, 2020
    @methane methane closed this as completed Jun 25, 2020
    @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
    3.10 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants