-
Notifications
You must be signed in to change notification settings - Fork 89
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
Conversation
…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.
There was a problem hiding this 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.
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Co-authored-by: Sam Brew <[email protected]>
Thanks for the feedback :) |
… disable_engine_compression to disable_bundle_compression, compress only for deploy and not on debug build
There was a problem hiding this 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!
I've just resolved a tiny conflict (Git being very silly), and, once tests are passing, this should be good to merge! Thanks again! |
There was a problem hiding this 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!
Relates to #199
Goal 1: Serve brotli pre compressed perseus engine files
Result :
On the basic example:
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
Result:
On the basic example:
I would be happy to discuss the changes 😄 , and explain the choices that I made