aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

Add Brotli compression provider #342

Closed khellang closed 6 years ago

khellang commented 6 years ago

Probably needs a few more tests, like for ordering. Should the Brotli provider be added by default?

Closes https://github.com/aspnet/BasicMiddleware/issues/217

khellang commented 6 years ago

Any pointers on tests you'd like to see?

Tratcher commented 6 years ago

Can you recycle some of the existing Flush tests? And at least one that shows what happens when the accept-encoding header includes gzip and br.

khellang commented 6 years ago

And at least one that shows what happens when the accept-encoding header includes gzip and br.

Is the intention that gzip will be preferred over Brotli and that you have to explicitly configure it if you want it the other way around?

Tratcher commented 6 years ago

Right

khellang commented 6 years ago

I guess this will also fix https://github.com/aspnet/BasicMiddleware/issues/216, as I had to take the "registration" order into account when selecting a compression provider when you have multiple matching providers with the same (or missing) quality value.

khellang commented 6 years ago

@Tratcher I added some benchmarks and ran them on the dev branch (before) and this branch (after) and they look pretty similar:

Before


BenchmarkDotNet=v0.10.14, OS=macOS High Sierra 10.13.4 (17E202) [Darwin 17.5.0]
Intel Core i7-4870HQ CPU 2.50GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.1.300
  [Host]     : .NET Core 2.1.1 (CoreCLR 4.6.26606.02, CoreFX 4.6.26606.05), 64bit RyuJIT
  Job-BPKSVL : .NET Core 2.1.1 (CoreCLR 4.6.26606.02, CoreFX 4.6.26606.05), 64bit RyuJIT

Runtime=Core  Server=True  Toolchain=.NET Core 2.1  
RunStrategy=Throughput  
Method AcceptEncoding Mean Error StdDev Op/s Gen 0 Allocated
GetCompressionProvider * 1.660 us 0.0092 us 0.0086 us 602,399.9 0.0076 1.52 KB
GetCompressionProvider br, compress, gzip 2.145 us 0.0149 us 0.0139 us 466,270.2 0.0076 1.71 KB
GetCompressionProvider gzip, compress 1.862 us 0.0089 us 0.0079 us 537,140.7 0.0095 1.64 KB
GetCompressionProvider gzip, compress, br 2.085 us 0.0197 us 0.0184 us 479,597.7 0.0076 1.71 KB
GetCompressionProvider gzip;q=0.8, compress;q=0.6, br;q=0.4 2.193 us 0.0171 us 0.0152 us 456,060.6 0.0076 1.71 KB
GetCompressionProvider identity 1.499 us 0.0093 us 0.0082 us 667,231.6 0.0076 1.52 KB

After


BenchmarkDotNet=v0.10.14, OS=macOS High Sierra 10.13.4 (17E202) [Darwin 17.5.0]
Intel Core i7-4870HQ CPU 2.50GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.1.300
  [Host]     : .NET Core 2.1.1 (CoreCLR 4.6.26606.02, CoreFX 4.6.26606.05), 64bit RyuJIT
  Job-YFUDVB : .NET Core 2.1.1 (CoreCLR 4.6.26606.02, CoreFX 4.6.26606.05), 64bit RyuJIT

Runtime=Core  Server=True  Toolchain=.NET Core 2.1  
RunStrategy=Throughput  
Method AcceptEncoding Mean Error StdDev Op/s Gen 0 Allocated
GetCompressionProvider * 1.582 us 0.0092 us 0.0081 us 632,169.5 0.0095 1.64 KB
GetCompressionProvider br, compress, gzip 2.052 us 0.0124 us 0.0116 us 487,361.5 0.0114 1.73 KB
GetCompressionProvider gzip, compress 1.569 us 0.0092 us 0.0082 us 637,229.3 0.0076 1.48 KB
GetCompressionProvider gzip, compress, br 2.066 us 0.0173 us 0.0153 us 484,111.3 0.0076 1.73 KB
GetCompressionProvider gzip;q=0.8, compress;q=0.6, br;q=0.4 2.193 us 0.0159 us 0.0133 us 456,019.0 0.0076 1.73 KB
GetCompressionProvider identity 1.304 us 0.0087 us 0.0081 us 766,637.0 0.0076 1.44 KB
khellang commented 6 years ago

Anything more I can do to get this over the finish line 🏁? 👼

Tratcher commented 6 years ago

Just waiting on a feature sign-off from @shirhatti. He'll be back Monday.

khellang commented 6 years ago

Rebased and swapped defaults 👌🏻

Tratcher commented 6 years ago

Sorry, I just realized we'd swapped the branches under you and this was targeting 3.0 (master). I changed the base branch back to 2.2 since we want this sooner than later 😁, but unfortunately that requires one more rebase. I can get to it later today if you don't.

khellang commented 6 years ago

Rebased. Hopfully it went OK 🙏

Tratcher commented 6 years ago

Great, thanks.

khellang commented 6 years ago

Wee! Thank you 😄

shirhatti commented 6 years ago

By adding brotli as the default we might have broken anyone running on full Fx. Whoops

Tratcher commented 6 years ago

We do not add brotli on net461.