refactor: Extract zlib-related logic into a single module #7223

mykola-mokhnach · 2023-02-24T16:39:08Z

What do these changes do?

Addresses issue #7192
Refactors the logic to have the zlib-related stuff concentrated into a single module

Are there changes in behavior for the user?

No

Related issue number

#7192

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_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

aiohttp/http_parser.py

aiohttp/zlib_utils.py

aiohttp/http_parser.py

aiohttp/zlib_utils.py

codecov · 2023-02-24T19:53:46Z

Codecov Report

Merging #7223 (307eafe) into master (3058c72) will decrease coverage by 0.03%.
The diff coverage is 88.67%.

@@            Coverage Diff             @@
##           master    #7223      +/-   ##
==========================================
- Coverage   97.33%   97.31%   -0.03%     
==========================================
  Files         106      107       +1     
  Lines       31309    31352      +43     
  Branches     3891     3901      +10     
==========================================
+ Hits        30476    30511      +35     
- Misses        634      640       +6     
- Partials      199      201       +2

Flag	Coverage Δ
CI-GHA	`97.22% <88.67%> (-0.03%)`	⬇️
OS-Linux	`96.87% <88.67%> (-0.03%)`	⬇️
OS-Windows	`95.30% <88.67%> (-0.02%)`	⬇️
OS-macOS	`96.46% <88.67%> (-0.03%)`	⬇️
Py-3.10.10	`96.98% <88.67%> (-0.03%)`	⬇️
Py-3.11.0	`96.40% <88.67%> (-0.03%)`	⬇️
Py-3.7.15	`96.69% <88.67%> (-0.03%)`	⬇️
Py-3.7.9	`95.17% <88.67%> (-0.02%)`	⬇️
Py-3.8.10	`95.09% <88.67%> (-0.02%)`	⬇️
Py-3.8.16	`96.60% <88.67%> (-0.03%)`	⬇️
Py-3.9.13	`95.08% <88.67%> (-0.03%)`	⬇️
Py-3.9.16	`96.62% <88.67%> (-0.03%)`	⬇️
Py-pypy7.3.11	`94.13% <85.84%> (-0.03%)`	⬇️
VM-macos	`96.46% <88.67%> (-0.03%)`	⬇️
VM-ubuntu	`96.87% <88.67%> (-0.03%)`	⬇️
VM-windows	`95.30% <88.67%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
aiohttp/web_response.py	`97.93% <75.00%> (-0.49%)`	⬇️
aiohttp/compression_utils.py	`84.61% <84.61%> (ø)`
aiohttp/client_reqrep.py	`97.72% <100.00%> (ø)`
aiohttp/http_parser.py	`98.28% <100.00%> (+0.78%)`	⬆️
aiohttp/http_websocket.py	`99.20% <100.00%> (+<0.01%)`	⬆️
aiohttp/http_writer.py	`99.15% <100.00%> (ø)`
aiohttp/multipart.py	`96.13% <100.00%> (-0.02%)`	⬇️
tests/test_http_parser.py	`99.08% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aiohttp/zlib_utils.py

for more information, see https://pre-commit.ci

aiohttp/web_response.py

aiohttp/http_parser.py

for more information, see https://pre-commit.ci

patchback · 2023-03-07T19:56:55Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 3ff81dc on top of patchback/backports/3.9/3ff81dc9c9ce20efd3bf54cf52adaf438c483a92/pr-7223

Backporting merged PR #7223 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/3ff81dc9c9ce20efd3bf54cf52adaf438c483a92/pr-7223 upstream/3.9

Now, cherry-pick PR refactor: Extract zlib-related logic into a single module #7223 contents into that branch:
```
$ git cherry-pick -x 3ff81dc9c9ce20efd3bf54cf52adaf438c483a92
```
If it'll yell at you with something like fatal: Commit 3ff81dc9c9ce20efd3bf54cf52adaf438c483a92 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 3ff81dc9c9ce20efd3bf54cf52adaf438c483a92
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR refactor: Extract zlib-related logic into a single module #7223 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/3ff81dc9c9ce20efd3bf54cf52adaf438c483a92/pr-7223

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

mykola-mokhnach · 2023-03-07T20:01:50Z

@Dreamsorcerer Thanks for checking and fine-tuning the PR. I appreciate your help

Dreamsorcerer · 2023-03-07T20:04:17Z

Thanks a lot. If you could handle the backport as well, that'd be great.

Addresses issue #7192 Refactors the logic to have the zlib-related stuff concentrated into a single module No #7192 - [x] I think the code is well written - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit 3ff81dc)

)  Backport #7223 Addresses issue #7192 Refactors the logic to have the zlib-related stuff concentrated into a single module No #7192 - [x] I think the code is well written - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit 3ff81dc)  ## What do these changes do?  ## Are there changes in behavior for the user?  ## Related issue number  ## 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: Mykola Mokhnach <mokhnach@gmail.com>

…n changes (#7797)  ## What do these changes do? Changes the threshold that is required to compress in the executor for websocket messages to 5KiB #7223 changed the websocket implementation to compress messages > 1KiB in the executor. The threshold was a bit low which caused an increase in latency compressing messages as the overhead to use the executor can exceed the cost to compress tiny messages. When testing 3.9.0 with Home Assistant, we saw a 3 order of magnitude increase in executor usage which resulted in an overall increase in cpu time since all the tiny messages were being compressed in the executor. I could not find the motivation for choosing 1KiB in the original PR ## Are there changes in behavior for the user?  ## Related issue number  ## Checklist - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

…n changes (#7797)  ## What do these changes do? Changes the threshold that is required to compress in the executor for websocket messages to 5KiB #7223 changed the websocket implementation to compress messages > 1KiB in the executor. The threshold was a bit low which caused an increase in latency compressing messages as the overhead to use the executor can exceed the cost to compress tiny messages. When testing 3.9.0 with Home Assistant, we saw a 3 order of magnitude increase in executor usage which resulted in an overall increase in cpu time since all the tiny messages were being compressed in the executor. I could not find the motivation for choosing 1KiB in the original PR ## Are there changes in behavior for the user?  ## Related issue number  ## Checklist - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." (cherry picked from commit 27c308b)

…messages from websocket compression changes (#7799) **This is a backport of PR #7797 as merged into master (27c308b).**  ## What do these changes do? Changes the threshold that is required to compress in the executor for websocket messages to 5KiB #7223 changed the websocket implementation to compress messages > 1KiB in the executor. The threshold was a bit low which caused an increase in latency compressing messages as the overhead to use the executor can exceed the cost to compress tiny messages. When testing 3.9.0 with Home Assistant, we saw a 3 order of magnitude increase in executor usage which resulted in an overall increase in cpu time since all the tiny messages were being compressed in the executor. I could not find the motivation for choosing 1KiB in the original PR ## Are there changes in behavior for the user?  ## Related issue number  ## Checklist - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: J. Nick Koston <nick@koston.org>

…h small messages from websocket compression changes (aio-libs#7799) **This is a backport of PR aio-libs#7797 as merged into master (27c308b).**  Changes the threshold that is required to compress in the executor for websocket messages to 5KiB aio-libs#7223 changed the websocket implementation to compress messages > 1KiB in the executor. The threshold was a bit low which caused an increase in latency compressing messages as the overhead to use the executor can exceed the cost to compress tiny messages. When testing 3.9.0 with Home Assistant, we saw a 3 order of magnitude increase in executor usage which resulted in an overall increase in cpu time since all the tiny messages were being compressed in the executor. I could not find the motivation for choosing 1KiB in the original PR   - [x] 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_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: J. Nick Koston <nick@koston.org>

mykola-mokhnach requested a review from asvetlov as a code owner February 24, 2023 16:39

github-advanced-security bot found potential problems Feb 24, 2023

View reviewed changes

aiohttp/zlib_utils.py Fixed Show resolved Hide resolved

aiohttp/zlib_utils.py Fixed Show fixed Hide fixed

Dreamsorcerer reviewed Feb 28, 2023

View reviewed changes

aiohttp/zlib_utils.py Outdated Show resolved Hide resolved

mykola-mokhnach and others added 10 commits March 3, 2023 18:40

refactor: Extract zlib-related logic into a single module

fa479be

[pre-commit.ci] auto fixes from pre-commit.com hooks

1c80247

for more information, see https://pre-commit.ci

make mypy happier

e9f6c38

[pre-commit.ci] auto fixes from pre-commit.com hooks

18f0320

for more information, see https://pre-commit.ci

Fix line length

536b54b

Update tests

ce4e693

[pre-commit.ci] auto fixes from pre-commit.com hooks

401c01b

for more information, see https://pre-commit.ci

Make mypy happy

1dac3f7

fix defaults

04dba2f

Remove extra types

573ee76

mykola-mokhnach force-pushed the zlib branch from 3396c46 to 573ee76 Compare March 3, 2023 17:40

Dreamsorcerer reviewed Mar 3, 2023

View reviewed changes

aiohttp/web_response.py Show resolved Hide resolved

Delete the obsolete method

c7f7578

Dreamsorcerer reviewed Mar 3, 2023

View reviewed changes

aiohttp/http_parser.py Outdated Show resolved Hide resolved

mykola-mokhnach and others added 10 commits March 3, 2023 19:25

Address comments

beb6c90

[pre-commit.ci] auto fixes from pre-commit.com hooks

b08e461

for more information, see https://pre-commit.ci

address review comments

829bab6

[pre-commit.ci] auto fixes from pre-commit.com hooks

4b274c3

for more information, see https://pre-commit.ci

Tune tests

a65547d

[pre-commit.ci] auto fixes from pre-commit.com hooks

e796fe9

for more information, see https://pre-commit.ci

Make mypy happy

9257884

[pre-commit.ci] auto fixes from pre-commit.com hooks

4b85a90

for more information, see https://pre-commit.ci

Update compression_utils.py

d9f3c49

Update compression_utils.py

570ffce

Dreamsorcerer and others added 3 commits March 7, 2023 18:14

Update multipart.py

662dfcb

Update compression_utils.py

96b35a6

[pre-commit.ci] auto fixes from pre-commit.com hooks

307eafe

for more information, see https://pre-commit.ci

Dreamsorcerer added bot:chronographer:skip This PR does not need to include a change note backport-3.9 labels Mar 7, 2023

Dreamsorcerer merged commit 3ff81dc into aio-libs:master Mar 7, 2023

mykola-mokhnach deleted the zlib branch March 7, 2023 20:01

mykola-mokhnach restored the zlib branch March 7, 2023 20:01

mykola-mokhnach deleted the zlib branch March 7, 2023 20:01

mykola-mokhnach restored the zlib branch March 7, 2023 20:01

mykola-mokhnach deleted the zlib branch March 7, 2023 20:01

Dreamsorcerer mentioned this pull request Apr 8, 2023

refactor: Extract zlib-related logic into a single module (#7223) #7250

Merged

10 tasks

This was referenced Nov 7, 2023

** 3.9 Beta testing ** #7675

Closed

Fix increase in latency with small messages from websocket compression changes #7797

Merged

patchback bot mentioned this pull request Nov 7, 2023

[PR #7797/27c308b1 backport][3.9] Fix increase in latency with small messages from websocket compression changes #7799

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Extract zlib-related logic into a single module #7223

refactor: Extract zlib-related logic into a single module #7223

mykola-mokhnach commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

patchback bot commented Mar 7, 2023 •

edited

Loading

mykola-mokhnach commented Mar 7, 2023

Dreamsorcerer commented Mar 7, 2023

refactor: Extract zlib-related logic into a single module #7223

refactor: Extract zlib-related logic into a single module #7223

Conversation

mykola-mokhnach commented Feb 24, 2023 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

patchback bot commented Mar 7, 2023 • edited Loading

Backport to 3.9: 💔 cherry-picking failed — conflicts found

Backporting merged PR #7223 into master

mykola-mokhnach commented Mar 7, 2023

Dreamsorcerer commented Mar 7, 2023

mykola-mokhnach commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

patchback bot commented Mar 7, 2023 •

edited

Loading