Skip to content

Conversation

@XhmikosR
Copy link
Contributor

Since Cloudflare is being used, Brotli compression is used.

Closes #2429

Draft until @rvagg feels the Brotli switch is fine

@rvagg
Copy link
Member

rvagg commented Aug 20, 2019

👍 up to the website team, it looks right to me:

$ curl -IH 'Accept-encoding: gzip,br,deflate'  https://nodejs.org/static/images/logo.svg
HTTP/2 200
date: Tue, 20 Aug 2019 11:53:16 GMT
content-type: image/svg+xml
...
content-encoding: br

I guess that's all good. Don't let me hold it up.

@silverwind
Copy link
Contributor

silverwind commented Aug 21, 2019

Which compression level does CF use? If it's not 11/9 for brotli/gzip, there could still be gains had by precompressing assets (I do have a module for that).

@XhmikosR
Copy link
Contributor Author

That I don't know TBH. I don't think they use the best compression, though, since it's slow.

I tried https://tools.paulcalvano.com/compression.php and it seems it's 5.

@silverwind
Copy link
Contributor

silverwind commented Aug 21, 2019

@rvagg can you link the nginx.conf in use? I think it might use gzip_static which is generally preferable for performance compared to on-the-fly compression like CF does, e.g. gzip static level 9 ought to beat brotli level 5 on-the-fly.

Also, if nginx has a brotli module loaded, we could use brotli_static, but it's not part of the regular nginx distribution. On Ubuntu, I use this nginx ppa which brings ngx_http_brotli_static_module.so.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Aug 21, 2019

For what is worth, the diff in TTFB is ~100ms AFAICT. EDIT: Which might be due to something else, I can't tell for sure myself.

https://gtmetrix.com/compare/csIu6DJG/s4fDLhlk

So, if the server conf supports serving precompressed assets, we should drop this PR and instead use your solution @silverwind to generate both br and gzip assets.

@silverwind
Copy link
Contributor

silverwind commented Aug 21, 2019

The server uses gzip_static as seen here. Ideally, we would introduce ngx_http_brotli_static_module.so and then adjust the gzip script to also generate .br files.

Now I do wonder how Cloudflare handles the case of the source server serving brotli, I think it needs to be checked, not that the work is in vain if Cloudflare compresses again regardless.

@XhmikosR
Copy link
Contributor Author

@silverwind how do you want to proceed with this?

@nschonni nschonni requested a review from silverwind October 16, 2019 22:27
@XhmikosR
Copy link
Contributor Author

@silverwind friendly ping

@XhmikosR
Copy link
Contributor Author

Just so that we are safe, @rvagg is the gzip script used in any deployment scripts which are not part of this repo?

@rvagg
Copy link
Member

rvagg commented Oct 30, 2019

Yes, it's used by the next line, you need to remove it from there too. npm run deploy is what's executed.
You can push to master and watch the log at https://nodejs.org/github-webhook.log to see how it goes. If it fails it'll let you know there and you can fix with a new push to master.

Since Cloudflare is being used, Brotli compression is used.
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 30, 2019

Oops, that was a rebase error, fixed now.

Thanks for the webhook info, I wasn't aware it existed.

It seems we are building node-sass each time :/ I guess #2724 might help in this regard.

@XhmikosR XhmikosR marked this pull request as ready for review October 30, 2019 08:22
@XhmikosR
Copy link
Contributor Author

@silverwind ping

@silverwind
Copy link
Contributor

silverwind commented Nov 13, 2019

I'm unsure here. If we keep gzip it looks like this (assuming cloudflare supports retrieving gzip):

origin (gzip) -> cloudflare (un-gzip + brotli) -> user

If we remove gzip:

origin (uncompressed) -> cloudflare (brotli) -> user

The first method saves bandwidth, while the second one saves cpu time. Ideal case would of course be serving brotli+gzip from the source, but that needs a non-default nginx module.

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

On second thought, I guess with proper caching, these origin -> cloudflare transfers probably only happen when files change, so it's probably better overall to remove it and shave of a few seconds from the deploy runtime.

@XhmikosR
Copy link
Contributor Author

Thanks @silverwind. @rvagg your thoughts?

@rvagg
Copy link
Member

rvagg commented Nov 15, 2019

fine by me I suppose

@XhmikosR
Copy link
Contributor Author

Alright, let's merge this and if there's anything unusual @rvagg we can revert it.

@XhmikosR XhmikosR merged commit 44bbff2 into nodejs:master Nov 20, 2019
@XhmikosR XhmikosR deleted the master-xmr-rm-gzip branch November 20, 2019 08:11
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.

is npm gzip script still useful?

4 participants