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

Feature Idea: Brotli support in core #18964

Closed
MylesBorins opened this issue Feb 23, 2018 · 25 comments
Closed

Feature Idea: Brotli support in core #18964

MylesBorins opened this issue Feb 23, 2018 · 25 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@MylesBorins
Copy link
Contributor

Taking a quick peek at "can i use" shows that brotli is now supported on Edge, Firefox, Chrome, Safari, and iOS Safari.

The iltorb package has 912k downloads a month.
The brotli npm package has 250k download a month

If people are open to this I would be willing to collaborate on a patch to add this functionality.

@addaleax addaleax added the feature request Issues that request new features to be added to Node.js. label Feb 23, 2018
@addaleax
Copy link
Member

Sounds like a good idea to me. I’m somewhat familiar with iltorbs code base, but the brotli API has also gotten to a point where it’s similar enough to zlib that we probably wouldn’t even have to write much code to integrate it into Node.

/cc @MayhemYDG

@devsnek
Copy link
Member

devsnek commented Feb 23, 2018

+1

p.s. if we're adding more compression stuff in core perhaps we should put it under one builtin module name require('compression').zlib/brotli/whateverelsecomesnext

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Feb 23, 2018

to be honest this may be the point where we decide if we want to use a nodejs namespace... as there is already a compression lib with 9M downloads a month and as mentioned above the brotli lib has 250k downloads a month. That being said I think it is a separate conversation that can be worked through independently

@nstepien
Copy link
Contributor

I think it makes sense. IMO Node, as a web server, should strive to support low-level modern web features, much like http2.

@silverwind
Copy link
Contributor

Yes, please. Brotli usage in Node.js has been held back by the barrier of native modules, this would certainly allow for broader adoption.

@MylesBorins
Copy link
Contributor Author

Pinging this to see if anyone is interested in taking this on as an action item

@vkurchatkin
Copy link
Contributor

It seems weird that "can I use" and download numbers are enough for adding brotli, but were never enough for adding websockets

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 13, 2018

@vkurchatkin tbh I think that we should revisit adding websockets.

If we look at the original thread #1010 some of the people who originally were -1 changed their opinions. In fact we ship a partial implementation of ws already, and have an open EPS exploring adding it nodejs/node-eps#6

The original decision by the TC at the time was to rather implement lower level buffer method, but that was abandoned.

#1202

I thinks I'll open another issue to discuss

edit:

issue for reference #19308

@MatonAnthony
Copy link
Contributor

Hello,

Which kind of skillset would it require? Would someone be willing to mentor me if I want to try it?

@jasnell
Copy link
Member

jasnell commented Mar 13, 2018

Quick question: why would this need to be added to core vs user land? Is there a notable perf improvement or impl improvement to be gained relative to the userland module?

@MylesBorins
Copy link
Contributor Author

This originally came up in a conversation with @kristoferbaxter where they mentioned having issues with userland implementations, and a lack of support in core being a reason for companies / enterprises to not adopt the format despite performance improvements compared to zlib.

I'm paraphrasing and will let them chime in

@kristoferbaxter
Copy link

kristoferbaxter commented Mar 13, 2018

Hi there.

There are a few userland implementations with differing drawbacks, but the most prominent are iltorb and wasm-brotli afaik.

iltorbhttps://github.com/MayhemYDG/iltorb
Uses prebuild to download a pre-compiled binary for your platform, if it exists. Otherwise, it will use node-gyp to build the module.

Pros: Works well for those who can leverage the pre-compiled binaries. Supports compression options.
Cons: Projects including it encounter issues when a pre-compiled binary is not available.

wasm-brotlihttps://github.com/dfrankland/wasm-brotli
WebAssembly compiled Brotli library.

Pros: No need for a pre-built binary.
Cons: Performance. Lacks compression options.


TLDR;
Since Brotli is quite well supported, can support runtime compression (using quality levels 4-6), and buildtime compression (using quality level 11), it is a prime candidate for use targeting modern browsers or native applications. Its inclusion in core would reduce friction to leverage it and hopefully make a meaningful difference in application performance for many usecases.

@jasnell
Copy link
Member

jasnell commented Mar 13, 2018

Aside from the convenience factor around native modules, are there specific performance or other ease of implementation considerations?

@kristoferbaxter
Copy link

kristoferbaxter commented Mar 13, 2018

Aside from the convenience factor around native modules

Convenience might be understating this. Every installation of iltorb is slower than other JS only modules, and has been problematic to install on some environments in my past experience. It's the main point for using something like wasm-brotli according to it's repo ("The awesome thing about wasm-brotli is that it does not need to compile or download any prebuilt binaries!").

Lowering the cost to try out brotli compression might allow a greater number of transmissions to be compressed using it versus gzip. For end users this can be a meaningful difference.

are there specific performance considerations?

The specific performance concerns mostly relate to usage of wasm-brotli versus iltorb. These performance concerns may not be significant enough to warrant the change, but metrics like this leave me concerned.

## payload size: 1024
iltorb (native compress) x 244 ops/sec ±0.94% (78 runs sampled)
wasm-brotli (rust wasm compress) x 5.21 ops/sec ±1.64% (29 runs sampled)
Fastest is iltorb (native compress)

are there specific implementation considerations?

The API of iltorb seems straight forward in my opinion (supports async, sync, streams and compression options) but there are likely others who can weigh in better.


Let's flip the question around a bit too, if common compression schemes like brotli should not be in core... should gzip implementations? Should http2? The larger question of what should be in core is hard to answer. I'm certainly not qualified to answer it, but am happy to share my experiences.

Edit: Added link for performance data. Sorry for updating without saying what was updated!

@jasnell
Copy link
Member

jasnell commented Mar 13, 2018

Cool thank you for the details. Please don't take my questions as push back, just need to build the case for the addition

@kristoferbaxter
Copy link

No worries, never came across that way to me!

Thanks for taking a look and hopefully this helps to explain a bit more why this might be a good idea.

@MatonAnthony
Copy link
Contributor

If I understand this properly, we could bind to the V8implementation of Brotli?

(I'm sorry if my question is stupid, all of this is quite new to me)

@hashseed
Copy link
Member

V8 doesn't include Brotli. Chromium does.

But yeah would be interesting to see how wasm performs.

Also, hi @MayhemYDG!

@MatonAnthony
Copy link
Contributor

Oh ok, so it's writing our own binding to Node. Is there a standard way or an example I could read about this?

@hashseed
Copy link
Member

Maybe take a look at how zlib binding is done?

@hashseed
Copy link
Member

Also, there is already one. https://github.com/MayhemYDG/iltorb

@bnoordhuis
Copy link
Member

There's an open pull request now, #20458. I'm curious if anyone has usage numbers of brotli on the internet.

I've been tracking my own internet usage but it's almost exclusively *.google.com that sends brotli back. Guess that makes sense with it being a Google thing.

@edmorley
Copy link

edmorley commented May 2, 2018

I'm curious if anyone has usage numbers of brotli on the internet.

Using the BigQuery query mentioned here (see the BigQuery guide here), there were 33K domains found using Brotli. For comparison that linked discussion post found only 357 domains when run in November 2016.

I've exported the top 500 domains by frequency (it refused to export the full list) to:
https://docs.google.com/spreadsheets/d/1bGPXJNUsaeJNYYgqUZQjW_rOzveHkzNbGyVBNoLm4E0/edit?usp=sharing

(For anyone wanting to repeat the above, the query only uses 5.8GB of BigQuery analysis allowance, and the first 1TB per month is free.)

@bnoordhuis
Copy link
Member

For anyone else wondering, that means just shy of 7% of the top 500k websites serve up brotli. More than I'd expected.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 6, 2019

@MylesBorins, @addaleax, #24938 landed, can this be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.