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

feat(http-compression): first version #3819

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

hhromic
Copy link
Contributor

@hhromic hhromic commented May 11, 2020

Summary

This PR implements HTTP compression in Portainer using both, on-the-fly compression and pre-compressed asset files serving.

Automatic asset files compression during client build is also implemented in Webpack.

Fixes #3638

Details

  • feat(http-compression): first version of on-the-fly HTTP compression
    • can be enabled with --compression flag (disabled by default)
    • uses gzip compression with default compression level (6)
    • only compresses content >= 1400 bytes (gziphandler library default)
  • feat(http-compression): first version of pre-compressed asset files serving
    • always enabled as it has almost no performance cost
    • automatically serves pre-compressed asset files if found on disk
    • recognises .br (Brotli) and .gz (gzip) compressed file extensions
  • feat(http-compression): added support for asset compression in Webpack
    • automatically compresses assets using CompressionWebpackPlugin
    • creates pre-compressed versions of assets alongside uncompressed files
    • uses gzip compression with high compression level (9)

Benchmarks

The following illustrates the transferred content size and time taken with and without requesting HTTP compression of the static asset files. These requests are local machine to local machine.

Main CSS asset

$ time curl -s http://localhost:9999/main.d0d9ade597a8341b903f.css | wc -c
23675
real    0m0.014s

$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/main.d0d9ade597a8341b903f.css | wc -c
4739
real    0m0.013s

Main JS asset

$ time curl -s http://localhost:9999/main.d0d9ade597a8341b903f.js | wc -c
3174340
real    0m0.019s

$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/main.d0d9ade597a8341b903f.js | wc -c
399610
real    0m0.014s

Vendor CSS asset

$ time curl -s http://localhost:9999/vendor.vendor.css | wc -c
276391
real    0m0.013s

$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/vendor.vendor.css | wc -c
49978
real    0m0.013s

Vendor JS asset

$ time curl -s http://localhost:9999/vendor.d0d9ade597a8341b903f.js | wc -c
6633504
real    0m0.022s

$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/vendor.d0d9ade597a8341b903f.js | wc -c
1356719
real    0m0.016s

SVG asset

$ time curl -s http://localhost:9999/0cb5a5c0d251c109458c85c6afeffbaa.svg | wc -c
715890
real    0m0.017s

$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/0cb5a5c0d251c109458c85c6afeffbaa.svg | wc -c
241513
real    0m0.014s

On slow networks (for example via VPN links), the loading times are significantly reduced when using gzip compression. Note how the biggest content (vendor JavaScript bundle) gets significantly reduced.

@hhromic
Copy link
Contributor Author

hhromic commented May 11, 2020

I just realised that during my testing using yarn build the assets don't get minified but in production they do. This means that the provided benchmark does not apply for production artifacts that are already compressed.

@hhromic
Copy link
Contributor Author

hhromic commented May 11, 2020

I benchmarked again with minified assets and gzip compression is still beneficial :)

# plain content, JavaScript data (vendor code)
$ curl -s http://localhost:9999/vendor.b9a69c1f6b66dbf34d09.js | wc -c
1839354
real    0m0.015s

# gzip compression, JavaScript data (vendor code)
$ curl -s -H "Accept-Encoding: gzip" http://localhost:9999/vendor.b9a69c1f6b66dbf34d09.js | wc -c
539318
real    0m0.106s

The good news is that the minified content has much less latency than non-minified and the compression is still good (~29%).


Minified Plain Content (~4.5 secs total time)

image

Minified Gzip compression (~1.7 secs total time)

image

@deviantony
Copy link
Member

Implementation is looking good, I like it.

I'm not sure if there is an interest in providing this via a flag, IMO we should decide that it's either gonna be beneficial for most or all of our userbase and enable the feature directly. Or maybe to default to gzip enabled with a way to disable it.

I think we'll need to do some testing here and we'll need to deploy it in multiple environments to see if it make any difference. On one side the loading time from 4.5sec to 1.5sec sounds great, on the other side the vendor code loading time going from 0m0.015s to 0m0.106s kinda worries me (~ 7x the original time).

I'm also curious about existing deployments that are using a reverse proxy setup + gzip enabled on the reverse proxy layer, what will happen here?

@hhromic
Copy link
Contributor Author

hhromic commented May 11, 2020

Implementation is looking good, I like it.

Thanks, I'm very new to GoLang so I appreciate the comment :)

I'm not sure if there is an interest in providing this via a flag, IMO we should decide that it's either gonna be beneficial for most or all of our userbase and enable the feature directly. Or maybe to default to gzip enabled with a way to disable it.

Gzip compression is usually a trade-off between network speed and compression speed. Not every network/server-machine combination is the same and thus enabling it or not depends on each user's conditions and preference. This is the reason for almost every web server that I know (many from this list) to implement gzip compression as an opt-in instead of opt-out. I would strongly advise on following the same practice in Portainer.

I think we'll need to do some testing here and we'll need to deploy it in multiple environments to see if it make any difference.

Yes! Definitively! My local testing only represents one case and if you have the machinery to test in multiple environments that's much better. I don't expect gzip compression to help in every case because of the mentioned trade-off.

On one side the loading time from 4.5sec to 1.5sec sounds great, on the other side the vendor code loading time going from 0m0.015s to 0m0.106s kinda worries me (~ 7x the original time).

Yes, again, the trade-off between network speed vs compression speed. In general slower networks + strong CPUs will show much better performance because despite taking more time for compressing, the total time compression + transmission is still less than original transmission as shown before. In my case due to the current world-wide situation I'm forced to work much more using my company's VPN in which gzip compression makes a significant difference.

If really interested on better performance, there is another option to consider as well: compress the assets on disk with gzip and serve them in this format directlly, thus avoiding re-compression over and over of the same static content. This eliminates the compression latency of big assets such as the vendor javascript/CSS.

I'm also curious about existing deployments that are using a reverse proxy setup + gzip enabled on the reverse proxy layer, what will happen here?

In deployments where there is a proxy in front of Portainer doing gzip compression and even SSL termination, then gzip compression in Portainer itself shoud not be used. Like with SSL, gzip compression should be moved entirely to the reverse proxy. This is another reason to have a flag for gzip compression and default it to false.

The main reason for me to implement this feature in Portainer is precisely to cater for these deployments that do not use reverse proxies in front of Portainer. In this case, and similar to the built-in SSL options, gzip compression comes handy and is very simple to enable.


Take your time to test and check this PR more carefully as you need. There is no rush! Any questions/suggestions are very welcome too.

@hhromic
Copy link
Contributor Author

hhromic commented May 11, 2020

what will happen here?

I realised that I did not answer the question directly. What happens here depends on the reverse proxy. Some might decompress the content from Portainer and re-compress again (dumber proxies), others might be smarter and pass-through the already compressed data.

@ghost ghost self-requested a review May 12, 2020 04:15
@hhromic
Copy link
Contributor Author

hhromic commented May 12, 2020

If really interested on better performance, there is another option to consider as well: compress the assets on disk with gzip and serve them in this format directlly, thus avoiding re-compression over and over of the same static content. This eliminates the compression latency of big assets such as the vendor javascript/CSS.

I just found out a Go module that implements that suggestion: https://github.com/lpar/gzipped

It is a drop-in replacement for the FileServer handler and plays nicely with gziphandler so we get the best of both worlds: pass-trough compressed files (i.e. static assets) or on-the-fly compressed data (i.e. dynamic JSON responses).

The only caveat is that it doesn't support directory indexes so I'll check if Portainer requires it. I will experiment with it a bit and let you know if it's worth it 👍.

@deviantony
Copy link
Member

Nice, keep us posted !

@hhromic
Copy link
Contributor Author

hhromic commented May 12, 2020

First prototype testing shows (as expected) good results.

In this benchmark, files are pre-compressed on disk (this can be done during deployment in Yarn/Grunt):

-rw-r--r--. 1 root root 2953957 May 12 09:46 main.7b942c935272cd8dcf21.js
-rw-r--r--. 1 root root  373981 May 12 09:46 main.7b942c935272cd8dcf21.js.gz
-rw-r--r--. 1 root root 6632330 May 12 09:46 vendor.7b942c935272cd8dcf21.js
-rw-r--r--. 1 root root 1356687 May 12 09:46 vendor.7b942c935272cd8dcf21.js.gz

I'm using the assets without minification here but as shown before after minification there is still further compression gain anyway.

Then Portainer picks the correct available file according to requested compression without having to repeatedly compress on-the-fly the static assets:

# plain content, vendor code (Javascript)
$ time curl -s http://localhost:9999/vendor.7b942c935272cd8dcf21.js | wc -c
6632330
real    0m0.023s

# gzip compressed, vendor code (Javascript)
$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/vendor.7b942c935272cd8dcf21.js | wc -c
1356687
real    0m0.015s

# plain content, main code (Javascript)
$ time curl -s http://localhost:9999/main.7b942c935272cd8dcf21.js | wc -c
2953957
real    0m0.016s

# gzip compressed, main code (Javascript)
$ time curl -s -H "Accept-Encoding: gzip" http://localhost:9999/main.7b942c935272cd8dcf21.js | wc -c
373981
real    0m0.014s

Because the only difference between these two is now only transmission time (no compression time for static assets), the compressed request is now always better than the uncompressed version.

I will polish the implementation and send another commit to this PR with this.

@hhromic
Copy link
Contributor Author

hhromic commented May 13, 2020

Just a heads-up, I submitted a PR yesterday to upstream lpar/gzipped to fix a minor compliance issue so it integrates nicely with Portainer. As soon as it is merged I will update this PR with the new funcionality for on-disk compressed assets so you can take a look.

@deviantony
Copy link
Member

deviantony commented May 13, 2020

@hhromic no rush, the next release where this might be integrated will be 2.0 (which is planned for release end of June).

@hhromic
Copy link
Contributor Author

hhromic commented May 14, 2020

Sure! No rush from my side either, I def want to implement this feature correctly and well-tested.
Thanks for giving it a chance in the first place 👍

* can be enabled with `--compression` flag (disabled by default)
* uses gzip compression with default compression level (6)
* only compresses content >= 1400 bytes (gziphandler library default)
…erving

* always enabled as it has almost no performance cost
* automatically serves pre-compressed asset files if found on disk
* recognises `.br` (Brotli) and `.gz` (gzip) compressed file extensions
* automatically compresses assets using `CompressionWebpackPlugin`
* creates pre-compressed versions of assets alongside uncompressed files
* uses gzip compression with high compression level (9)
@pull-dog
Copy link

pull-dog bot commented May 15, 2020

Ruff 🐶 I am running in lazy mode (as per your pull-dog.json configuration file), so I won't start provisioning a test environment for this pull request until I hear from your build server 💤 Give it a few minutes, and check back.

React on this comment to leave anonymous feedback.

  • 👍 to say good dog 🍖
  • 👎 to say bad dog 🦴
Commands
  • /pull-dog go fetch to reprovision or provision the server.
  • /pull-dog get lost to delete the provisioned server.

@hhromic hhromic changed the title feat(gzip-compression): first version feat(http-compression): first version May 15, 2020
@hhromic
Copy link
Contributor Author

hhromic commented May 15, 2020

@deviantony @itsconquest
I fully finished a revised first version of this feature now. I edited the original PR description.

In summary, I implemented:

  • On-the-fly HTTP compression (disabled by default, enable with --compresion)
  • Pre-compressed asset files serving (always enabled, due to almost no CPU cost)
  • Automatic compression of static asset files in Webpack during client building

The automatic asset files compression does not replace uncompressed files, but instead places pre-compressed files alongside the uncompressed ones for maximum compatibility. For example:

main.d0d9ade597a8341b903f.css
main.d0d9ade597a8341b903f.css.gz
main.d0d9ade597a8341b903f.js
main.d0d9ade597a8341b903f.js.gz
vendor.d0d9ade597a8341b903f.js
vendor.d0d9ade597a8341b903f.js.gz
vendor.vendor.css
vendor.vendor.css.gz

The on-the-fly HTTP compression is mostly useful for large dynamic JSON responses from Portainer.

This PR is now ready for you to test and review. I'll be looking forward for your comments and feedback. In my local tests the feature is working really nice with reduced content transfers.

As said before, no rush from my side! Hope you find this a nice feature.

@deviantony
Copy link
Member

Great job @hhromic ! I'll add this to our review pipe for 2.0

@ghost
Copy link

ghost commented May 20, 2020

Hope you find this a nice feature

Solid PR, I look forward to reviewing this

Copy link
Contributor

@yi-portainer yi-portainer left a comment

Choose a reason for hiding this comment

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

Please also resolve the merge conflicts.

@@ -13,11 +16,21 @@ type Handler struct {
// NewHandler creates a handler to serve static files.
func NewHandler(assetPublicPath string) *Handler {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a "compression" boolean flag here, so that only uses gzipped when compression is needed?

@yi-portainer
Copy link
Contributor

Great job for the PR @hhromic, much appreciated! Before merging the PR, can you please resolve the conflicts and add a boolean parameter for the file handle to honor the cli "compression" flag? Thank you.

@hhromic
Copy link
Contributor Author

hhromic commented Aug 12, 2020

Hi @yi-portainer, thanks for your feedback. I will try to come back to this at my earliest.
I used to have my own Portainer "dev" container to develop for Portainer but I lost it. I saw there is an official dev workflow from you guys based on Docker now? Could you point me to any new information? Thanks!

@yi-portainer
Copy link
Contributor

Hi @yi-portainer, thanks for your feedback. I will try to come back to this at my earliest.
I used to have my own Portainer "dev" container to develop for Portainer but I lost it. I saw there is an official dev workflow from you guys based on Docker now? Could you point me to any new information? Thanks!

We had some very good initiatives in this PR, if a containerized dev environment is what you are looking for? Do note that it is still in experiment and not yet ready for day to day consumption (hence not officially documented yet), so please be careful when playing with it.

@ghost ghost removed their request for review February 2, 2022 05:45
chiptus pushed a commit to chiptus/portainer that referenced this pull request Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HTTP gzip compression in internal Portainer server
3 participants