aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

Use io.BytesIO() instead of List[bytes] + b"".join()

Open atzannes opened this issue 9 months ago • 6 comments

What do these changes do?

Provisional PR to check that the solution proposed to issue #10550 doesn't break anything else

Are there changes in behavior for the user?

No user-level changes. This is intended to result in improved memory allocation / deallocation

Is it a substantial burden for the maintainers to support this?

No

Related issue number

#10550

Checklist

  • [ ] I think the code is well written
  • [ ] Unit tests for the changes exist
  • [ ] Documentation reflects the changes
  • [ ] If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • [ ] Add a new news fragment into the CHANGES/ folder
    • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

    • if you don't have an issue number, change it to the pull request number after creating the PR

      • .bugfix: A bug fix for something the maintainers deemed an improper undesired behavior that got corrected to match pre-agreed expectations.
      • .feature: A new behavior, public APIs. That sort of stuff.
      • .deprecation: A declaration of future API removals and breaking changes in behavior.
      • .breaking: When something public is removed in a breaking way. Could be deprecated in an earlier release.
      • .doc: Notable updates to the documentation structure or build process.
      • .packaging: Notes for downstreams about unobvious side effects and tooling. Changes in the test invocation considerations and runtime assumptions.
      • .contrib: Stuff that affects the contributor experience. e.g. Running tests, building the docs, setting up the development environment.
      • .misc: Changes that are hard to assign to any of the above categories.
    • Make sure to use full sentences with correct case and punctuation, for example:

      Fixed issue with non-ascii contents in doctest text files
      -- by :user:`contributor-gh-handle`.
      

      Use the past tense or the present tense a non-imperative mood, referring to what's changed compared to the last released version of this project.

atzannes avatar Mar 16 '25 21:03 atzannes

CodSpeed Performance Report

Merging #10570 will degrade performances by 45.58%

Comparing atzannes:master (a6969b4) with master (f76cab7)

Summary

❌ 3 regressions
✅ 44 untouched benchmarks

:warning: Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_one_hundred_get_requests_with_30000_chunked_payload[pyloop] 40.5 ms 46.1 ms -12.12%
test_one_hundred_get_requests_with_30000_content_length_payload[pyloop] 39.4 ms 45.1 ms -12.6%
test_one_hundred_get_requests_with_512kib_content_length_payload[pyloop] 164.7 ms 302.6 ms -45.58%

codspeed-hq[bot] avatar Mar 16 '25 21:03 codspeed-hq[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.71%. Comparing base (45b861f) to head (a6969b4). Report is 60 commits behind head on master.

:white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10570      +/-   ##
==========================================
+ Coverage   98.69%   98.71%   +0.01%     
==========================================
  Files         122      125       +3     
  Lines       37230    37367     +137     
  Branches     2064     2064              
==========================================
+ Hits        36745    36885     +140     
+ Misses        338      335       -3     
  Partials      147      147              
Flag Coverage Δ
CI-GHA 98.58% <100.00%> (+0.01%) :arrow_up:
OS-Linux 98.25% <100.00%> (+<0.01%) :arrow_up:
OS-Windows 96.18% <100.00%> (-0.01%) :arrow_down:
OS-macOS 97.36% <100.00%> (+<0.01%) :arrow_up:
Py-3.10.11 97.27% <100.00%> (+<0.01%) :arrow_up:
Py-3.10.16 97.82% <100.00%> (+0.01%) :arrow_up:
Py-3.11.11 97.89% <100.00%> (+<0.01%) :arrow_up:
Py-3.11.9 97.35% <100.00%> (+<0.01%) :arrow_up:
Py-3.12.9 98.35% <100.00%> (+<0.01%) :arrow_up:
Py-3.13.2 98.34% <100.00%> (+<0.01%) :arrow_up:
Py-3.9.13 97.15% <100.00%> (-0.01%) :arrow_down:
Py-3.9.21 97.68% <100.00%> (+<0.01%) :arrow_up:
Py-pypy7.3.16 81.87% <100.00%> (-7.36%) :arrow_down:
VM-macos 97.36% <100.00%> (+<0.01%) :arrow_up:
VM-ubuntu 98.25% <100.00%> (+<0.01%) :arrow_up:
VM-windows 96.18% <100.00%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 16 '25 21:03 codecov[bot]

Tests seem to pass, but benchmarks show a drop in performance. Maybe that's why the list was used originally.

Dreamsorcerer avatar Mar 16 '25 22:03 Dreamsorcerer

I think the problem in https://github.com/aio-libs/aiohttp/issues/10550 is that everything is being done in a single read. If you read in chunks there it might fix the problem you are having

bdraco avatar Mar 16 '25 22:03 bdraco

I think if memory is an issue, a user should be using the streaming API. So, it's probably better here to optimise for CPU rather than memory.

Dreamsorcerer avatar Mar 16 '25 22:03 Dreamsorcerer

Thank you for the feedback. I'll try to address the performance issue

atzannes avatar Mar 16 '25 23:03 atzannes