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

enable compression for js files in Nextcloud #2576

Merged
merged 3 commits into from
May 30, 2023
Merged

Conversation

szaimen
Copy link
Collaborator

@szaimen szaimen commented May 20, 2023

Close #2574

@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request labels May 20, 2023
@szaimen szaimen added this to the next milestone May 20, 2023
Containers/apache/Caddyfile Outdated Show resolved Hide resolved
@Zoey2936
Copy link
Collaborator

but why do you only want to enable it for JavaScript files?

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

but why do you only want to enable it for JavaScript files?

Because I don't want to risk any possible side-effects. E.g. I could imagine that you might run into problems when the desktop client is not prepared for this kind of encoding on webdav responses and I actually also don't want to find out currently XD

@Zoey2936
Copy link
Collaborator

but why do you only want to enable it for JavaScript files?

Because I don't want to risk any possible side-effects. E.g. I could imagine that you might run into problems when the desktop client is not prepared for this kind of encoding on webdav responses and I actually also don't want to find out currently XD

Not sure... compression (gzip and also brotli) is basically a default in the internet

@Zoey2936
Copy link
Collaborator

Zoey2936 commented May 20, 2023

But beside this, what about the mastercontainer and/or enabling compression in apache (mastercontainer/apache conatiner)? https://httpd.apache.org/docs/2.4/mod/mod_deflate.html https://httpd.apache.org/docs/2.4/mod/mod_brotli.html (both are inside the httpd:alpine image, not sure if they are inside the alpine package repo)

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

But beside this, what about the mastercontainer and/or enabling compression in apache (mastercontainer/apache conatiner)? https://httpd.apache.org/docs/2.4/mod/mod_deflate.html https://httpd.apache.org/docs/2.4/mod/mod_brotli.html (both are inside the httpd:alpine image, not sure if they are inside the alpine package repo)

Since the mastercontainer uses mostly server-side rendering, I don't think that it needs compression.

Enabling compression in Apache would only make sense if it comes with an additional size benefit but since gzip already reduces the size from 13.3MB to 3.5 MB, I don't think there is a lot of benefit using Apache for content-encoding.

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

Just verified, brotli even decreses it to 2.0 MB so it makes sense. Also since we enable this in apache, it only enables it for Nextcloud which should limit side-effects.

@szaimen szaimen force-pushed the enh/2574/content-encoding branch from 7e84bcf to 54e0067 Compare May 20, 2023 22:34
@szaimen szaimen changed the title enable content encoding for javascript files enable brotli compression for js files in Nextcloud May 20, 2023
@Zoey2936
Copy link
Collaborator

do you only want to add brotli support? maybe also support gzip/deflate as fallback

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

do you only want to add brotli support? maybe also support gzip/deflate as fallback

How would I add gzip as fallback?

@Zoey2936
Copy link
Collaborator

Enable the deflate module and use the same configuartion, but for deflate/gzip, but maybe brotli is enough, all modern browsers support it now: https://caniuse.com/brotli

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

but maybe brotli is enough, all modern browsers support it now: https://caniuse.com/brotli

indeed, nice!

@szaimen szaimen force-pushed the enh/2574/content-encoding branch 2 times, most recently from 5535681 to f15aa0a Compare May 20, 2023 23:38
@Zoey2936
Copy link
Collaborator

Another question, why does nextcloud not provide precompressed (gzip/brotli) files? that would be nice, since then the webserver does not need to compress files and can just sent the precompressed files, the only thing is that they are in a binary format?

@szaimen
Copy link
Collaborator Author

szaimen commented May 20, 2023

Another question, why does nextcloud not provide precompressed (gzip/brotli) files? that would be nice, since then the webserver does not need to compress files and can just sent the precompressed files, the only thing is that they are in a binary format?

I wonder the same and asked this in nextcloud/server#36728 (comment)

@szaimen szaimen changed the title enable brotli compression for js files in Nextcloud enable compression for js files in Nextcloud May 21, 2023
@szaimen
Copy link
Collaborator Author

szaimen commented May 21, 2023

Apparentl, brotly compression on the fly is slow: nextcloud/server#36728 (comment) so I guess reverting back to gzip is the way to go for now.

@Zoey2936
Copy link
Collaborator

Zoey2936 commented May 21, 2023

Not sure: https://blog.cloudflare.com/results-experimenting-brotli even the lowest compression level of brotli (0) seems to be better/the same as the strongest of gzip and has the same speed then gzip level 4, in my NPM fork I decided to use gzip level 1 and brotli level 0.

@szaimen
Copy link
Collaborator Author

szaimen commented May 21, 2023

Apparently the problem is rather that compressing on the fly on the server side is pretty slow. Better would be if they would be pre-compressed. However gzip compression on the fly is faster iirc.

@szaimen
Copy link
Collaborator Author

szaimen commented May 21, 2023

Ill make some tests the coming days.

@szaimen
Copy link
Collaborator Author

szaimen commented May 22, 2023

I just tested this with the login page (time with new load in new private window until it shows login form) and these are the results:
(manually tracked, so possible that they are not 100% correct but I did the test multiple times to confirm that they are not completely incorrect).

~4.12s loading time without compression
~2.76s loading time with Brotli
~3.34s loading time with Deflate

So even on the fly Brotli seems to be the fastest :)

@Zoey2936
Copy link
Collaborator

what compression level did you used?

@szaimen
Copy link
Collaborator Author

szaimen commented May 22, 2023

what compression level did you used?

4, the same like configured in this PR

@Zoey2936
Copy link
Collaborator

can you try with a compression level of 0 (brotli) and 1 (gzip), I got with the lowest level the best result, because higher level could cause high cpu usage, if there many requests at the same time

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

See #2793

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

but why js not?

I think the reason lies here: https://github.com/nextcloud/server/blob/bd3f3afcf715ee283f61a45e4a26e566f949a7a5/lib/private/AppFramework/Middleware/CompressionMiddleware.php#L64-L78 and here: https://github.com/nextcloud/server/blob/bd3f3afcf715ee283f61a45e4a26e566f949a7a5/apps/dav/lib/Connector/Sabre/PropfindCompressionPlugin.php#L64-L68

So apparently only responses that are served by Nextcloud via php are compressed. Files that are served via the web server are not.

And indeed this seems to be only javascript, css and svg files that are thus not compressed because every other plain filetype is apparently served via php.

@Zoey2936
Copy link
Collaborator

https://github.com/nextcloud/server/blob/bd3f3afcf715ee283f61a45e4a26e566f949a7a5/lib/private/Template/JSCombiner.php#L197 does this mean it is compressed using level 9? Is level 1 not better?

@Zoey2936
Copy link
Collaborator

and do you know if brotli is also supported by nc itself?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

and do you know if brotli is also supported by nc itself?

I dont think so

@Zoey2936
Copy link
Collaborator

https://github.com/nextcloud/server/blob/bd3f3afcf715ee283f61a45e4a26e566f949a7a5/lib/private/Template/JSCombiner.php#L197 does this mean it is compressed using level 9?

yes

should it maybe be set down to level 1?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

should it maybe be set down to level 1?

Possibly but it does not look like it is even used anymore....

@Zoey2936
Copy link
Collaborator

I thought nc does compression by itself under some conditions?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

In appdata, I only found these files:
image

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

It does not seem to be used apart from that...

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

image

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

Actually I think compression level 9 is okay for the jscombiner since it is only created once and then cached IIRC. However for the others there should indeed be a low compression level to keep the load low.

@Zoey2936
Copy link
Collaborator

I think this should be done, and it could also improve the performance a bit

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

As for the responses I think level 6 is used if not specified...

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

I think this should be done, and it could also improve the performance a bit

you mean for the compressionmiddleware and compressionplugin?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

Which level would you recommend for these two?

@Zoey2936
Copy link
Collaborator

As for the responses I think level 6 is used if not specified...

yes, default seems to be 6

@Zoey2936
Copy link
Collaborator

I think this should be done, and it could also improve the performance a bit

you mean for the compressionmiddleware and compressionplugin?

yes

@Zoey2936
Copy link
Collaborator

Which level would you recommend for these two?

not sure, needs testing, but I think it can be the lowest/a very low value

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

So level 1 for example? level 0 is no compression IIRC

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

However I read this in the zlib manual: Z_DEFAULT_COMPRESSION requests a default compromise between speed and compression (currently equivalent to level 6).
reducing to level 1 will reduce the load on the server but obviously also increase the size and load time. So not sure if it really makes sense to adjust the level?

@szaimen
Copy link
Collaborator Author

szaimen commented Jun 17, 2023

@Zoey2936
Copy link
Collaborator

However I read this in the zlib manual: Z_DEFAULT_COMPRESSION requests a default compromise between speed and compression (currently equivalent to level 6). reducing to level 1 will reduce the load on the server but obviously also increase the size and load time. So not sure if it really makes sense to adjust the level?

that is the reason why it think it should be tested

@philiprenich
Copy link

Hey, should I be seeing gzip'd assets coming through with the latest version of AIO? Status is merged, but I'm not sure about release cycles. I'm on AIO v7.2.1 which is Nextcloud Hub 6 and version 27.1.0
Would love to not have to download nearly 50mb for my dashboard 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants