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

WIP: Use go:embed with server-side gzip support #26533

Closed
wants to merge 2 commits into from

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Aug 16, 2023

Replace #22887

Without pre-compressing, the binary size is 20MB larger.

gitea-vfsgen-gzip 102M
gitea-embed 123M

This approach could also support pre-compressing, just wrap the fs.FS to add the gzip support. However, it's not that easy to make Gitea's asset code and go:embed work well together with the pre-compressed files. Maybe it needs a separate temp dir like "temp/public", "temp/templates" and "temp/options" to store the pre-comporessed files, and then let "go:embed" to pack the "temp" directory.


This PR is mainly for a demo purpose (PoC), since vfsgen doesn't bother me too much, so at the moment I wouldn't spend time on the "pre-compressing" feature.

Feel free to edit this PR directly or start a new one.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 16, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 16, 2023
@wxiaoguang wxiaoguang changed the title Use go:embed with server-side gzip support WIP: Use go:embed with server-side gzip support Aug 16, 2023
@wxiaoguang wxiaoguang marked this pull request as draft August 16, 2023 04:33
"code.gitea.io/gitea/modules/templates"
)

//go:embed options public templates modules/migration/schemas
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//go:embed options public templates modules/migration/schemas
//go:embed all:options all:public all:templates all:modules/migration/schemas

The all: prefix makes it embed files starting with _ and . too. I ran into this issue in another app where a file named like _.8538ed73.js would not embed. Currently webpack outputs a file -.8538ed73.js so it would still embed, but I think it's better safe to add this now instead of be surprised in the future.

@silverwind
Copy link
Member

silverwind commented Aug 16, 2023

I think its better to compress before embed, e.g. with a temp directory. That way we don't have to pay the runtime cost of compression and binary size will be optimal.

https://github.com/silverwind/precompress is one tool to do it, but I see it's not ideal because i outputs into the same directory as the source file. To output into a temp directory, a new output directory option will be necessary for it.

@wxiaoguang
Copy link
Contributor Author

This PR is mainly for a demo purpose (PoC), since vfsgen doesn't bother me too much, so at the moment I wouldn't spend time on the "pre-compressing" feature.

Feel free to edit this PR directly or start a new one.

@silverwind
Copy link
Member

silverwind commented Aug 16, 2023

I will work on outputting to a separate dir in precompress (leaving source directory structure intact), then I can try integrating it here.

I think we may be able to get rid of all the go generate steps in Makefile with this.

@wxiaoguang
Copy link
Contributor Author

No problem, then I will finish the "wrap the fs.FS to add the gzip support" part if there are compressed assets.

@silverwind
Copy link
Member

silverwind commented Aug 16, 2023

I guess the assets will be:

/embed/migration/${path}.gz
/embed/options/${path}.gz
/embed/public/${path}.gz
/embed/assets/${path}.gz

If we decide to switch to brotli with the decoder, it would be .br extension.

@wxiaoguang
Copy link
Contributor Author

/embed/migration/${path}.gz

To keep the consistent directory layout, IMO it could be /embed/modules/migration/schemas/...

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 16, 2023

If we decide to switch to brotli with the decoder, it would be .br extension.

No extension should be used. Because they should have the same file name as the "custom" directory.

Otherwise, the "List" function couldn't get the correct file list.

eg: List expects to see "/templates/head.tmpl" but not "(custom)/templates/head.tmpl" + "(embed)/templates/head.tmpl.gz"

@silverwind
Copy link
Member

Okay, so original filename, but compressed bytes within. A bit unconventional, but possible.

@silverwind
Copy link
Member

I've released https://github.com/silverwind/precompress v12, so in theory it should be:

npx precompress -t gz -E -o embed/assets -b public/assets public/assets

This should give compressed assets like embed/assets/js/index.js.

@silverwind
Copy link
Member

silverwind commented Aug 22, 2023

Will check later to contribute it to this PR. One thing we need to consider is that we should serve uncompressed files during development as the cost of compression would be too high during development for no real benefit, especially once it's switched to brotli.

Ideally I would still like to emit assets with a .gz extension, by the way. It's clearer and then you don't need to guess the encoding while serving.

@wxiaoguang
Copy link
Contributor Author

This PR is mainly for a demo purpose (PoC), since vfsgen doesn't bother me too much, so at the moment I wouldn't spend time on the "pre-compressing" feature.

Feel free to edit this PR directly or start a new one.

@wxiaoguang wxiaoguang closed this Dec 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants