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

Set websocket writer compression to Z_BEST_SPEED #4993

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Oct 6, 2020

What do these changes do?

The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.

Are there changes in behavior for the user?

This is a noticeable performance gain with many connections
and large message sizes. Compressed sizes will be slightly
smaller as described #2856 (comment)

Related issue number

#2856 (comment)

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."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 6, 2020
The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.
@bdraco bdraco force-pushed the websocket_compress_overhead branch from 2a85f71 to 413c02a Compare October 6, 2020 01:36
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2020

Codecov Report

Merging #4993 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4993   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files          43       43           
  Lines        8933     8933           
  Branches     1406     1406           
=======================================
  Hits         8720     8720           
  Misses         95       95           
  Partials      118      118           
Flag Coverage Δ
#unit 97.61% <100.00%> (ø)

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

Impacted Files Coverage Δ
aiohttp/http_websocket.py 98.64% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55e6497...c484b13. Read the comment docs.

@bdraco
Copy link
Member Author

bdraco commented Oct 6, 2020

Not sure if docs should be updated as the compression level isn't currently documented.

@bdraco
Copy link
Member Author

bdraco commented Oct 6, 2020

If this is acceptable, I'll make a backport PR to 3.6 as well

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@asvetlov asvetlov merged commit b66b36a into aio-libs:master Oct 15, 2020
patchback bot pushed a commit that referenced this pull request Oct 15, 2020
The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
(cherry picked from commit b66b36a)
github-actions bot pushed a commit that referenced this pull request Oct 15, 2020
The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
@github-actions
Copy link
Contributor

💚 Backport successful

The PR was backported to the following branches:

asvetlov pushed a commit that referenced this pull request Oct 15, 2020
The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
(cherry picked from commit b66b36a)

Co-authored-by: J. Nick Koston <nick@koston.org>
@asvetlov
Copy link
Member

@bdraco sorry, the time window for aiohttp 3.6 is closed.
The next release will be 3.7; I hope to publish it soon, after reviewing/merging pending pull requests.

asvetlov added a commit that referenced this pull request Oct 16, 2020
The performance overhead of using the defaults was
noticeable with many connections. This change
matches Nginx default.

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants