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

perf(web): optimize response sizes for initial page load #7594

Merged
merged 5 commits into from Mar 6, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Mar 3, 2024

Description

This PR tries to optimize the initial page load and generally reduce response sizes.

  • Making the AssetViewer a dynamic import since it's bulky and doesn't need to be loaded immediately
  • Enabling compression at the fastest setting (even the fastest dramatically reduces response size, and slower settings see diminishing returns)
  • Using webp instead of jpeg for Memory Lane

@mertalev mertalev changed the title perf(web): optimize initial page load perf(web): optimize initial page load response sizes Mar 3, 2024
@mertalev mertalev changed the title perf(web): optimize initial page load response sizes perf(web): optimize response sizes for initial page load Mar 3, 2024
Copy link

cloudflare-pages bot commented Mar 3, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5b8cbf0
Status: ✅  Deploy successful!
Preview URL: https://ac3f94b0.immich.pages.dev
Branch Preview URL: https://perf-web-minify.immich.pages.dev

View logs

server/src/immich/main.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

LGTM

@benmccann
Copy link
Contributor

I just found this old SvelteKit issue: sveltejs/kit#5431. We used to have compression built-in, but had to remove it because it was regularly causing the server to hang. Unfortunately the fix still hasn't been merged: expressjs/compression#183. And you can't provide the answer to the question that the PR is blocked on as the repository has blocked all outside contributors from commenting 😕

The precompress option might be safer. Ideally we'd be able to use precompress to aggressively precompress the assets in brotli and then do a quicker gzip compression with compress here for dynamic responses. But I don't know how much we want to chance the server randomly hanging. Maybe we should read through that old issue and see if there were certain circumstances that would cause it to trigger. I don't remember all the details. I'm guessing precompress would cover most requests though and should get us an even larger gain.

@mertalev
Copy link
Contributor Author

mertalev commented Mar 3, 2024

Oof, that sucks. I'll remove the compression for now until we take a closer look

@mertalev mertalev merged commit f883430 into main Mar 6, 2024
25 checks passed
@mertalev mertalev deleted the perf/web-minify branch March 6, 2024 17:05
@aviv926
Copy link
Contributor

aviv926 commented Mar 6, 2024

I just found this old SvelteKit issue: sveltejs/kit#5431. We used to have compression built-in, but had to remove it because it was regularly causing the server to hang. Unfortunately the fix still hasn't been merged: expressjs/compression#183. And you can't provide the answer to the question that the PR is blocked on as the repository has blocked all outside contributors from commenting 😕

The precompress option might be safer. Ideally we'd be able to use precompress to aggressively precompress the assets in brotli and then do a quicker gzip compression with compress here for dynamic responses. But I don't know how much we want to chance the server randomly hanging. Maybe we should read through that old issue and see if there were certain circumstances that would cause it to trigger. I don't remember all the details. I'm guessing precompress would cover most requests though and should get us an even larger gain.

It seems that this project is not going to receive a commitment soon, there is this project that received a commitment 3 days ago and a year ago and he is the one who offered the PR
which is not merged into main

Maybe you should keep an eye on it?
https://github.com/fernandolguevara/compression/tree/master

@benmccann
Copy link
Contributor

That's the fork that the fix (expressjs/compression#183) is coming from. It was called too hacky to merge as it relies on brittle Node internals, so I don't think we want to use it. The author of the PR wants to keep support for ancient versions of Node rather than address that, which means it will probably never be merged.

Unfortunately, all outside users are blocked from sending fixes, so it is likely to remain broken forever.

Screenshot from 2024-03-06 15-06-08

Perhaps we should build our own compression middleware into SvelteKit or polka or something.

@benmccann
Copy link
Contributor

There was a PR to polka that Vite copied and has been using the past couple of years. I'll see if we can't get that merged to use. See here: sveltejs/kit#9593 (comment)

@mertalev
Copy link
Contributor Author

mertalev commented Mar 7, 2024

That'd be awesome!

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.

None yet

5 participants