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

Add precompressed brotli/gzip assets #12398

Closed
wants to merge 9 commits into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Aug 1, 2020

This uses webpack to emit .gz and .br files for each js/css file into the public directory. The benefit of this is that our webpack-managed assets are now always optimally compressed without any runtime performance impact.

The backend will dynamically emit compressed public assets based on the client's content-encoding support and there is no interference with the existing gzip option (which does slow runtime compression).

Overall, this will increase bindata size and memory consumption when using bindata by around 20 MiB while saving around 25-50% of transfer size for every affected asset (in the case of brotli vs previous runtime gzip).

Node >= 10.16 is required for native brotli compression support.

@silverwind silverwind changed the title Add pre-compresses js/css assets Add precompressed brotli/gzip assets Aug 1, 2020
@silverwind silverwind force-pushed the precompress branch 5 times, most recently from 54b8dd0 to 95e9b8b Compare August 1, 2020 11:46
@lunny
Copy link
Member

lunny commented Aug 2, 2020

I think the bindata tool github.com/shurcooL/vfsgen has compressed the assets.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 2, 2020
@silverwind
Copy link
Member Author

silverwind commented Aug 2, 2020

vfsgen transparently compresses if the resulting file is smaller (which will only be the case for the uncompressed variants) but gitea will always see uncompressed data coming out of it. It has no influence on the http compression which is still our responsibilty.

This PR should be a big performance win for uncached asset downloads on both the case when runtime gzip is diabled (resulting in much smaller transfers) or when enabled (reducing cpu load on the server by not having to perform runtime compression).

@silverwind
Copy link
Member Author

silverwind commented Aug 2, 2020

It might be possible to access vfsgen's raw gzip data to save some bindata size but I think that's an optimization can be done in another PR. Brotli data needs to be stored in any case because vfsgen does not support it.

Also it seems vfsgen does not specify a compression level for gzip so will get default level 6 which is not ideal compression (which would be level 9) so I doubt its usefulness.

Edit: See these issues:

shurcooL/vfsgen#85
shurcooL/vfsgen#71

This uses webpack to emit .gz and .br files for each js/css file into
the public directory. The benefit of this is that our assets are now
always optimally compressed without any runtime performance impact.

The backend will dynamically emit compressed public assets based on
the client's content-encoding support and there is no inteference with
the existing gzip option (which does slow runtime compression).

Overall, this will increase bindata size and memory consumption by
around 20 MiB while saving around 25-50% of transfer size for every
affected asset.
@lafriks
Copy link
Member

lafriks commented Aug 3, 2020

Imho this should be done in startup with some kind of warmup method, this way it would not increase binary size and would still provide same performance

@silverwind
Copy link
Member Author

silverwind commented Aug 4, 2020

Brotli compression is rather cpu-intensive, it takes around 15 seconds on a quad-core machine. I guess https://github.com/andybalholm/brotli could be used, but I don't see an extra 20MB binary size as a big issue either. Once shurcooL/vfsgen#86 is merged and released, we can cut the space requirement in half for the bindata case.

@silverwind
Copy link
Member Author

silverwind commented Aug 4, 2020

Actually the 20mb was too generously estimated, it's less then that even:

$ find public -name '*.gz' -exec du -ch {} + | grep total$
4.0M    total
$ find public -name '*.br' -exec du -ch {} + | grep total$
3.0M    total

So we can either take that as-is or wait on above PR to pull gzip data from vfsgen and reduce the 7MB to just 3MB for the brotli assets. I still kind of like to have both the .br and .gz assets thought because web servers can directly pick those up (gzip_static and brotli_static directives in nginx for example).

Generally I think doing 15+ seconds of brotli compression on every startup is a bad idea as it might hog useful cpu resources.

@silverwind
Copy link
Member Author

silverwind commented Aug 4, 2020

Actually I think using the vfsgen gzip data will introduce a dependency on TAGS on the frontend build. If bindata is absent, we need to emit both .gz and .br, otherwise only .br (because vfsgen provides gzip).

@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Aug 5, 2020
@silverwind
Copy link
Member Author

silverwind commented Aug 24, 2020

shurcooL/vfsgen#86 is merged so we can probably soon use it to get level 9 gzip data out of it.

The question remains whether we should keep the brotli assets (currently 3MB) which will of course mean 3MB higher memory and disk space requirement for around 30% less transfer size for the assets compared to gzip. To me it's pretty clear that we should keep the brotli files.

@silverwind
Copy link
Member Author

Thought there's also a question whether we really need the gzip data. Pretty much all modern browser support Brotli, so I don't imagine gzip being used all that often.

@techknowlogick techknowlogick added this to the 1.14.0 milestone Sep 5, 2020
@stale stale bot added the issue/stale label Nov 6, 2020
@go-gitea go-gitea deleted a comment from stale bot Dec 23, 2020
@stale stale bot removed the issue/stale label Dec 23, 2020
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Dec 23, 2020
@lunny
Copy link
Member

lunny commented Dec 23, 2020

So could we send a PR to vfsgen to support Brotli?

@silverwind
Copy link
Member Author

Yes, I think that'd be ideal for us and the author seems willing, see shurcooL/vfsgen#71.

@silverwind
Copy link
Member Author

silverwind commented Dec 23, 2020

Closing, #7109 should replace it.

@silverwind silverwind closed this Dec 23, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants