MM-28491: Optimize gzip compression of responses by agnivade · Pull Request #15416 · mattermost/mattermost

agnivade · 2020-09-09T06:39:48Z

It looks like maintainers aren't very active on the repo
and nytimes/gziphandler#107 has come to a standstill.

I don't want to wait forever for this to go in. Let's use a replace directive
to point to the fork. When the PR gets merged, we can just bump the dependency
and remove the replace directive.

Re: the PR, copying the text from the PR description

This gives 40-50% improvements in CPU with very minor increase
in memory, just as a drop-in replacement. I think that is a very reasonable tradeoff.

The library is mature and safe to be used in production.

name                 old time/op    new time/op    delta
GzipHandler_S2k-8      74.9µs ± 2%    34.4µs ± 2%  -54.07%  (p=0.000 n=10+9)
GzipHandler_S20k-8      379µs ± 1%     226µs ± 3%  -40.42%  (p=0.000 n=9+10)
GzipHandler_S100k-8    1.95ms ± 2%    1.15ms ± 1%  -41.27%  (p=0.000 n=9+9)
GzipHandler_P2k-8      24.3µs ±25%    10.7µs ±25%  -55.80%  (p=0.000 n=10+10)
GzipHandler_P20k-8      132µs ± 2%      75µs ± 1%  -42.95%  (p=0.000 n=9+10)
GzipHandler_P100k-8     658µs ± 2%     371µs ± 3%  -43.68%  (p=0.000 n=9+10)

name                 old alloc/op   new alloc/op   delta
GzipHandler_S2k-8      7.71kB ± 5%    9.13kB ± 7%  +18.33%  (p=0.000 n=10+10)
GzipHandler_S20k-8     65.1kB ± 3%    70.3kB ± 3%   +8.05%  (p=0.000 n=10+10)
GzipHandler_S100k-8     348kB ± 4%     382kB ± 2%   +9.85%  (p=0.000 n=10+10)
GzipHandler_P2k-8      7.60kB ± 1%    7.93kB ± 2%   +4.33%  (p=0.000 n=10+10)
GzipHandler_P20k-8     64.4kB ± 1%    66.3kB ± 2%   +2.92%  (p=0.000 n=10+10)
GzipHandler_P100k-8     304kB ± 1%     309kB ± 1%   +1.67%  (p=0.000 n=10+9)

name                 old allocs/op  new allocs/op  delta
GzipHandler_S2k-8        21.0 ± 0%      21.0 ± 0%     ~     (all equal)
GzipHandler_S20k-8       24.0 ± 0%      24.0 ± 0%     ~     (all equal)
GzipHandler_S100k-8      27.0 ± 0%      27.0 ± 0%     ~     (all equal)
GzipHandler_P2k-8        21.0 ± 0%      21.0 ± 0%     ~     (all equal)
GzipHandler_P20k-8       24.0 ± 0%      24.0 ± 0%     ~     (all equal)
GzipHandler_P100k-8      26.0 ± 0%      26.0 ± 0%     ~     (all equal)

https://mattermost.atlassian.net/browse/MM-28491

It looks like maintainers aren't very active on the repo and nytimes/gziphandler#107 has come to a standstill. I don't want to wait forever for this to go in. Let's use a replace directive to point to the fork. When the PR gets merged, we can just bump the dependency and remove the replace directive. Re: the PR, copying the text from the PR description This gives 40-50% improvements in CPU with very minor increase in memory, just as a drop-in replacement. I think that is a very reasonable tradeoff. The library is mature and safe to be used in production. ``` name old time/op new time/op delta GzipHandler_S2k-8 74.9µs ± 2% 34.4µs ± 2% -54.07% (p=0.000 n=10+9) GzipHandler_S20k-8 379µs ± 1% 226µs ± 3% -40.42% (p=0.000 n=9+10) GzipHandler_S100k-8 1.95ms ± 2% 1.15ms ± 1% -41.27% (p=0.000 n=9+9) GzipHandler_P2k-8 24.3µs ±25% 10.7µs ±25% -55.80% (p=0.000 n=10+10) GzipHandler_P20k-8 132µs ± 2% 75µs ± 1% -42.95% (p=0.000 n=9+10) GzipHandler_P100k-8 658µs ± 2% 371µs ± 3% -43.68% (p=0.000 n=9+10) name old alloc/op new alloc/op delta GzipHandler_S2k-8 7.71kB ± 5% 9.13kB ± 7% +18.33% (p=0.000 n=10+10) GzipHandler_S20k-8 65.1kB ± 3% 70.3kB ± 3% +8.05% (p=0.000 n=10+10) GzipHandler_S100k-8 348kB ± 4% 382kB ± 2% +9.85% (p=0.000 n=10+10) GzipHandler_P2k-8 7.60kB ± 1% 7.93kB ± 2% +4.33% (p=0.000 n=10+10) GzipHandler_P20k-8 64.4kB ± 1% 66.3kB ± 2% +2.92% (p=0.000 n=10+10) GzipHandler_P100k-8 304kB ± 1% 309kB ± 1% +1.67% (p=0.000 n=10+9) name old allocs/op new allocs/op delta GzipHandler_S2k-8 21.0 ± 0% 21.0 ± 0% ~ (all equal) GzipHandler_S20k-8 24.0 ± 0% 24.0 ± 0% ~ (all equal) GzipHandler_S100k-8 27.0 ± 0% 27.0 ± 0% ~ (all equal) GzipHandler_P2k-8 21.0 ± 0% 21.0 ± 0% ~ (all equal) GzipHandler_P20k-8 24.0 ± 0% 24.0 ± 0% ~ (all equal) GzipHandler_P100k-8 26.0 ± 0% 26.0 ± 0% ~ (all equal) ``` https://mattermost.atlassian.net/browse/MM-28491

isacikgoz

LGTM, hope your PR can be merged very soon 🚀

streamer45

Given the simplicity of the implementation and the fact that the upstream has been idle for almost a year can't we just host our own version like we do for other things (e.g. gorp, viper)?
This is to avoid going through the same process in case we need additional changes in the future.

agnivade · 2020-09-09T12:41:12Z

Hmm .. yeah let's keep it under my username for now. If there are more changes, we can move it under mattermost.

agnivade · 2020-09-09T12:48:46Z

@lindalumitchell - can be qa-deferred.

lindalumitchell

Thanks!

agnivade · 2020-09-15T16:13:32Z

Testing mattermod. Please ignore.

/cherry-pick release-5.27

mattermod · 2020-09-15T16:13:34Z

Cherry pick is scheduled.

mattermod · 2020-09-15T16:13:36Z

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.112.3' to the list of known hosts (/app/.ssh/known_hosts).
From github.com:mattermost/mattermost-server
   2ef200c2c..a3aec838d  MM-27122       -> upstream/MM-27122
   e6229dfb3..d16bb45d2  MM-27353       -> upstream/MM-27353
   9f9eb0be7..96b949268  MM-27481_GetPluginStatus -> upstream/MM-27481_GetPluginStatus
   d008d88ad..ecaa8d51c  MM-27918       -> upstream/MM-27918
 * [new branch]          MM-28124       -> upstream/MM-28124
 * [new branch]          MM-28125       -> upstream/MM-28125
   015010df7..e68bcf085  MM-28210       -> upstream/MM-28210
   e35e6b309..d7615ee2d  MM-28303-better-gh-rate-limiting -> upstream/MM-28303-better-gh-rate-limiting
   2cfc1dec5..1b102a00c  MM-28383       -> upstream/MM-28383
 * [new branch]          MM-28454       -> upstream/MM-28454
 * [new branch]          MM-28475-Telemetry-admin-roles -> upstream/MM-28475-Telemetry-admin-roles
 * [new branch]          MM-28696       -> upstream/MM-28696
 * [new branch]          add-cloud-beta-pipeline -> upstream/add-cloud-beta-pipeline
   c1ac3e69f..7dfe24ee7  cloud-beta     -> upstream/cloud-beta
 * [new branch]          cloud-purchase -> upstream/cloud-purchase
   1058090a2..7abc4f538  master         -> upstream/master
   98b7d509b..6ffa286c1  mm-26420-add-wildcard-support-bleve -> upstream/mm-26420-add-wildcard-support-bleve
 * [new branch]          plugin-marketplace-arch -> upstream/plugin-marketplace-arch
   d98ed35df..bd6149b00  release-5.27   -> upstream/release-5.27
 * [new branch]          update-node-build-image -> upstream/update-node-build-image
 * [new tag]             v5.27.0        -> v5.27.0
 * [new tag]             v5.27.0-rc2    -> v5.27.0-rc2
Fetching origin
Failed to add the RSA host key for IP address '140.82.112.3' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
!!! 'upstream/mattermod.' not found. The second argument should be something like upstream/release-0.21.
    (In particular, it needs to be a valid, existing remote branch that I can 'git checkout'.)

+++ Returning you to the master branch and cleaning up.

agnivade · 2020-09-15T16:17:23Z

/cherry-pick release-5.27

mattermod · 2020-09-15T16:17:25Z

Cherry pick is scheduled.

mattermod · 2020-09-15T16:17:26Z

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.113.4' to the list of known hosts (/app/.ssh/known_hosts).
Fetching origin
Failed to add the RSA host key for IP address '140.82.114.4' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-server-#15416-upstream-release-5.27-1600186645
Switched to a new branch 'automated-cherry-pick-of-mattermost-server-#15416-upstream-release-5.27-1600186645'
Branch 'automated-cherry-pick-of-mattermost-server-#15416-upstream-release-5.27-1600186645' set up to track remote branch 'release-5.27' from 'upstream'.

+++ About to attempt cherry pick of PR #15416 with merge commit 96a3407b7a38cb8bb48f2d1ad95b4e3dbc324577.

error: could not apply 96a3407b7... Optimize gzip compression of responses (#15416)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

+++ Conflicts detected:

UU go.sum
UU vendor/modules.txt
Aborting.

+++ Aborting in-progress git cherry-pick.

+++ Returning you to the master branch and cleaning up.

agnivade added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 9, 2020

agnivade requested review from isacikgoz and streamer45 September 9, 2020 06:39

agnivade changed the title ~~Optimize gzip compression of responses~~ MM-28491: Optimize gzip compression of responses Sep 9, 2020

isacikgoz approved these changes Sep 9, 2020

View reviewed changes

streamer45 reviewed Sep 9, 2020

View reviewed changes

streamer45 approved these changes Sep 9, 2020

View reviewed changes

streamer45 removed the 2: Dev Review Requires review by a developer label Sep 9, 2020

agnivade requested a review from lindalumitchell September 9, 2020 12:48

lindalumitchell approved these changes Sep 9, 2020

View reviewed changes

lindalumitchell added QA - Deferred Agreement with QA that these changes will be tested post-merge 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 9, 2020

agnivade merged commit 96a3407 into master Sep 10, 2020

agnivade deleted the compress2 branch September 10, 2020 03:32

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-28491: Optimize gzip compression of responses#15416

MM-28491: Optimize gzip compression of responses#15416
agnivade merged 1 commit intomasterfrom
compress2

agnivade commented Sep 9, 2020

Uh oh!

isacikgoz left a comment

Uh oh!

streamer45 left a comment

Uh oh!

agnivade commented Sep 9, 2020

Uh oh!

agnivade commented Sep 9, 2020

Uh oh!

lindalumitchell left a comment

Uh oh!

agnivade commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

agnivade commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

agnivade commented Sep 9, 2020

Uh oh!

isacikgoz left a comment

Choose a reason for hiding this comment

Uh oh!

streamer45 left a comment

Choose a reason for hiding this comment

Uh oh!

agnivade commented Sep 9, 2020

Uh oh!

agnivade commented Sep 9, 2020

Uh oh!

lindalumitchell left a comment

Choose a reason for hiding this comment

Uh oh!

agnivade commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

agnivade commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

mattermod commented Sep 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants