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

Engine Brotli compressed and optional http servers compression #300

Merged
merged 17 commits into from
Dec 12, 2023

Conversation

JT117
Copy link
Contributor

@JT117 JT117 commented Jul 7, 2023

Relates to #199

Goal 1: Serve brotli pre compressed perseus engine files

  • Change perseus-cli to brotli compress the engine files after building them (maybe better to add it in the cmds vec ?)
  • Add a flag in the perseus-cli to deactivate the brotli compress file generation: disable_engine_compression
  • Change the servers configuration to serve the .br files first and if not found defaults to the uncompressed file.

Result :

On the basic example:

Mode File raw kB brotli kB
serve bundle.js 42.42 6.5
bundle.wasm 2980 554.71
deploy bundle.js 32.43 5.23
bundle.wasm 315.98 109.62

The lighthouse score on mobile goes up from 97% to 100%
On my app it goes from 89% to 100% on mobile.

Remarks: Each server has it's own way to do thing, it goes from simple to I had to make a new Responder for Rocket

Goal 2: Add the possibility for users to enable http compression on the servers

  • Add on actix-web, axum, rocket and warp a new features: dflt-server-with-compression
  • If the features is enabled, the server activate native compression

Result:

On the basic example:

Server File raw (kB) compressed (kB) Type
rocket index 1.010 0.638 brotli
about 0.927 0.328 brotli
warp index 1.010 0.657 gzip
about 0.927 0.611 gzip
actix-web index 1.010 0.694 gzip
about 0.927 0.647 gzip
axum index 1.010 0.657 gzip
about 0.927 0.610 gzip

I would be happy to discuss the changes 😄 , and explain the choices that I made

…hange the servers configuration to look for the .br files first and if not found defaults to the uncompressed file.

Changed the servers(actix-web, axum, rocket, warp) configuration to add a dflt-server-with-compression feature that enables native server compression.

By default, brotli compress the perseus_engine.js, perseus_engine.wasm, perseus_engine.d.ts and perseus_engine_bg.wasm.d.ts.
This behaviour can be deactivated with the disable_engine_compression flag on the perseus CLI.
Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Thank you so much for all this, it's really impressive! The benchmarks speak for themselves I think, and this is definitely something I want to get into Perseus sooner rather than later: my only big question is about the Rocket implementation, because it seems like a lot of code for something that's fairly simple in the other servers. Could you explain why all this is necessary? (I've tried to keep the server integrations as simple as possible, is there perhaps some way you could think of refactoring the core so we minimise the amount of code for the servers?)

As for having compression in the CLI have its own spinner, how long have you observed compression taking for the examples? Also, I would suggest having the dflt-server-with-compression flag activate a function with a unique name so the features aren't mutually exclusive (who knows, there may be some exotic configuration using both, but I don't see any reason to force the user into only having one). Also, I recommend changing disable_engine_compression to disable_bundle_compression to make what that flag does clearer by name in the CLI. I've left some more detailed minor suggestions (whitespace etc.) in my review.

Also, do you want to move the compression process in the CLI to the deployment system? That would avoid unnecessarily slowing down development builds (which generate multi-MB Wasm bundles, compression of which seems pointless and slow), and it would also make sure you compress the post-minification JS bundle.

packages/perseus-actix-web/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-actix-web/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-actix-web/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-axum/Cargo.toml Outdated Show resolved Hide resolved
packages/perseus-axum/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-rocket/Cargo.toml Outdated Show resolved Hide resolved
packages/perseus-rocket/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-rocket/src/pre_compressed_named_file.rs Outdated Show resolved Hide resolved
packages/perseus-warp/Cargo.toml Outdated Show resolved Hide resolved
packages/perseus-warp/src/lib.rs Outdated Show resolved Hide resolved
@JT117
Copy link
Contributor Author

JT117 commented Oct 6, 2023

Thanks for the feedback :)
I'm on it, I will look again at rocket to see if I missed something, compress only the js and wasm bundle, change the flag name to
disable_bundle_compression and accept all the corrections you made

… disable_engine_compression to disable_bundle_compression, compress only for deploy and not on debug build
@JT117 JT117 requested a review from arctic-hen7 October 6, 2023 16:03
Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

This is looking really fantastic, thank you so much for all your hard work, it's greatly appreciated!

@arctic-hen7
Copy link
Member

I've just resolved a tiny conflict (Git being very silly), and, once tests are passing, this should be good to merge! Thanks again!

Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Fabulous, thanks so much for all this!

@arctic-hen7 arctic-hen7 merged commit 8016599 into framesurge:main Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants