Add Brotli compression provider #342

khellang · 2018-06-26T14:53:59Z

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

Closes #217

Tratcher · 2018-06-26T15:58:12Z

src/Microsoft.AspNetCore.ResponseCompression/BrotliCompressionProvider.cs

+        public string EncodingName => "br";
+
+        /// <inheritdoc />
+        public bool SupportsFlush => false;


Did you verify this? We would like flush support.
It appears to be supported:
https://github.com/dotnet/corefx/blob/master/src/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs#L110

Oh, I tried to find it in the source, but looked at a different file (without .Compress)🤦🏻‍♂️

Tratcher · 2018-06-26T16:04:29Z

src/Microsoft.AspNetCore.ResponseCompression/BrotliCompressionProviderOptions.cs

+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+#if NETCOREAPP2_1


The public types should be NETSTANDARD, let's keep the NETCOREAPP regions as constrained as possible. You should be able to limit them to ResponseCompressionProvider.ctor and BrotliCompressionProvider.CreateStream

But what will they do on non-netcoreapp2.1? Throw?

CreateStream is the only place that would need to throw (NotImplementedException, NotSupportedException?). And that exception will be avoided by having ResponseCompressionProvider skip adding the provider. It would only throw if someone added it manually.

It would only throw if someone added it manually.

Good point 👍🏻

I'm assuming that also means that you're fine with adding Brotli as a default provider?

That's fine, as long as it's second on the list.

Tratcher · 2018-06-26T16:06:50Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

+                    new CompressionProviderFactory(typeof(GzipCompressionProvider)),
+#if NETCOREAPP2_1
+                    new CompressionProviderFactory(typeof(BrotliCompressionProvider))
+#endif


List all TFMs and include the else and error cases so we know if these need updating.

Agreed on the error case, but what should the other TFMs do?

The other TFMs can be empty.

khellang · 2018-06-26T16:51:37Z

src/Microsoft.AspNetCore.ResponseCompression/BrotliCompressionProvider.cs

+        public Stream CreateStream(Stream outputStream)
+        {
+#if NET461
+            throw new PlatformNotSupportedException();


Do these require a message?

I'm not worried about it. I don't expect people to hit this.
You can combine net461 and netstandard2_0.

Not sure if that would work. I think you need to check NETCOREAPP2_1 first, then check NETSTANDARD2_0

Nvrm, I didn't realize ifdefs are not inherited.

khellang · 2018-06-26T21:42:13Z

Any pointers on tests you'd like to see?

Tratcher · 2018-06-26T23:24:22Z

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 · 2018-06-26T23:34:30Z

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 · 2018-06-26T23:55:36Z

Right

khellang · 2018-06-27T11:14:54Z

I guess this will also fix #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.

Tratcher

This is looking really good. I wonder about the repetitive complexity and if we need fast paths or caching for the common case.
@shirhatti is doing some analysis of other implementations to see if the priority system needs to be more flexible. We'll follow up with you.

Tratcher · 2018-06-27T15:42:37Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

+                    var encodingName = encoding.Value;
+                    var quality = encoding.Quality.GetValueOrDefault(1);
+
+                    if (Math.Abs(quality) < double.Epsilon)


Why the Abs?

Uh, I blame R# 🤣

Tratcher · 2018-06-27T15:50:48Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

                        {
-                            return provider;
+                            candidates.Add(new ProviderCandidate(provider.EncodingName, quality, i, provider));


This is starting to become a lot of work on every response, and you'd get the same results most of the time, no? e.g. gzip, deflate, br or gzip, deflate are always going to evaluate to the same provider. Is it worth caching a few results based on the raw header string to avoid this redundant processing? Or at least having a fast path for headers that do not contain quality, then you can do a Contains for each available provider and be done.

I don't think it's much more work, it's just doing it the other way around by matching first and ordering after, but we can certainly try to be smarter to avoid some work 👍🏻

Might be worth checking/considering making _providers a Dictionary? Besides that, I don't see much you can do.

Tratcher · 2018-06-27T16:06:17Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

                    }
                }
+
+                if (candidates.Count == 0)


could be combine with ElementAtOrDefault(0)

Tratcher · 2018-06-27T16:10:22Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

+
+            public double Quality { get; }
+
+            public int Order { get; }


LOL, I had Priority and changed it to Order last minute 😂

Tratcher · 2018-06-27T16:17:25Z

test/Microsoft.AspNetCore.ResponseCompression.Tests/ResponseCompressionMiddlewareTest.cs

+        [Theory]
+        [InlineData("gzip", "br")]
+        [InlineData("br", "gzip")]
+        public async Task Request_AcceptMixed_CompressedGzip(string encoding1, string encoding2)


I don't see the opposite test where you configure the providers in the other order and ensure br is selected. (NETCOREAPP2_2 only)

Yeah, that's still missing. I'm reluctant to copy-pasting the setup code more, but I'm not sure if there there's a good way to reuse the setup as there are quite a few permutations.

khellang · 2018-06-27T21:12:11Z

@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

jkotalik

Looking clean.

jkotalik · 2018-07-06T06:22:58Z

src/Microsoft.AspNetCore.ResponseCompression/BrotliCompressionProvider.cs

+#if NETCOREAPP2_1
+            return new BrotliStream(outputStream, Options.Level, leaveOpen: true);
+#elif NET461 || NETSTANDARD2_0
+            throw new PlatformNotSupportedException();


Let's add a comment to the exception.

A comment explaining why we throw, or an exception message?

Exception message 😄

jkotalik · 2018-07-06T06:35:31Z

src/Microsoft.AspNetCore.ResponseCompression/BrotliCompressionProviderOptions.cs

+namespace Microsoft.AspNetCore.ResponseCompression
+{
+    /// <summary>
+    /// Options for the <see cref="BrotliCompressionProvider"/>


Relating to options, can you add a comment to ResponseCompressionOptions saying the Providers's priority is based on their ordering in the list?

jkotalik · 2018-07-06T06:39:15Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

                    {
-                        // No compression
-                        return null;
+                        candidates.Add(new ProviderCandidate(encodingName.Value, quality, int.MaxValue, null));


We should add a comment here about what each of these values mean.

The arguments?

Yeah for the arguments. Or add named params.

jkotalik · 2018-07-06T06:41:20Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

                        {
-                            return provider;
+                            candidates.Add(new ProviderCandidate(provider.EncodingName, quality, i, provider));


Might be worth checking/considering making _providers a Dictionary? Besides that, I don't see much you can do.

jkotalik · 2018-07-06T06:43:27Z

.../Microsoft.AspNetCore.ResponseCompression.Benchmarks/ResponseCompressionProviderBenchmark.cs

@@ -0,0 +1,54 @@
+using System.Collections.Generic;


Nit: copyright header.

jkotalik · 2018-07-06T06:43:38Z

benchmarks/Microsoft.AspNetCore.ResponseCompression.Benchmarks/AssemblyInfo.cs

@@ -0,0 +1 @@
+[assembly: BenchmarkDotNet.Attributes.AspNetCoreBenchmark]


Nit: copyright header.

khellang · 2018-07-06T08:05:27Z

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

Tratcher · 2018-07-06T16:34:03Z

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

Tratcher

Please rebase.

Tratcher · 2018-07-09T17:58:44Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs

+                {
+                    new CompressionProviderFactory(typeof(GzipCompressionProvider)),
+#if NETCOREAPP2_1
+                    new CompressionProviderFactory(typeof(BrotliCompressionProvider))


We decided to make Brotli higher priority than Gzip. We'll have time to evaluate the impact in the preview releases. Please switch the order and then we can get this merged.

khellang · 2018-07-10T15:01:50Z

Rebased and swapped defaults 👌🏻

Tratcher · 2018-07-10T16:21:02Z

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 · 2018-07-10T19:37:22Z

Rebased. Hopfully it went OK 🙏

Tratcher · 2018-07-10T19:44:16Z

Great, thanks.

khellang · 2018-07-10T19:48:28Z

Wee! Thank you 😄

shirhatti · 2018-07-11T01:09:12Z

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

Tratcher · 2018-07-11T01:14:38Z

We do not add brotli on net461.

Tratcher reviewed Jun 26, 2018

View reviewed changes

khellang force-pushed the brotli-provider branch 2 times, most recently from 920f84f to f89a2be Compare June 26, 2018 16:50

khellang commented Jun 26, 2018

View reviewed changes

khellang force-pushed the brotli-provider branch 4 times, most recently from c51e066 to 170b08a Compare June 26, 2018 18:55

khellang force-pushed the brotli-provider branch 2 times, most recently from 31062f8 to 48a2160 Compare June 27, 2018 11:12

Tratcher reviewed Jun 27, 2018

View reviewed changes

khellang force-pushed the brotli-provider branch from 1c8f224 to 026932b Compare June 27, 2018 21:33

natemcmaster changed the base branch from dev to master July 2, 2018 17:44

jkotalik reviewed Jul 6, 2018

View reviewed changes

khellang force-pushed the brotli-provider branch 2 times, most recently from 3474e3e to af8bec1 Compare July 6, 2018 07:58

Tratcher reviewed Jul 9, 2018

View reviewed changes

khellang force-pushed the brotli-provider branch from af8bec1 to 17ec376 Compare July 10, 2018 12:48

Tratcher approved these changes Jul 10, 2018

View reviewed changes

Tratcher changed the base branch from master to release/2.2 July 10, 2018 16:19

khellang force-pushed the brotli-provider branch 2 times, most recently from 66d9783 to cb26243 Compare July 10, 2018 19:35

khellang added 11 commits July 10, 2018 21:35

Added failing test for Brotli compression

f1005aa

Added Brotli compression provider

3023146

Add Brotli to existing test cases

9cd9a79

Add failing test for accept encoding order

d7fada4

Use compression provider order when selecting provider

812bdab

Some test cleanup

8ad00c3

PR feedback

852d22c

Added benchmarks for GetCompressionProvider

c275fb6

Added Brotli configuration order test

298a2cd

PR feedback

c1689b7

Switch Brotli and Gzip priority

91068a5

khellang force-pushed the brotli-provider branch from cb26243 to 91068a5 Compare July 10, 2018 19:36

Tratcher merged commit 767b3ef into aspnet:release/2.2 Jul 10, 2018

khellang deleted the brotli-provider branch July 10, 2018 19:48

guardrex mentioned this pull request Jul 20, 2018

Brotli compression dotnet/AspNetCore.Docs#7715

Closed

		@@ -0,0 +1 @@
		[assembly: BenchmarkDotNet.Attributes.AspNetCoreBenchmark]

Add Brotli compression provider #342

Add Brotli compression provider #342

Uh oh!

Conversation

khellang commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khellang commented Jun 26, 2018

Uh oh!

Tratcher commented Jun 26, 2018

Uh oh!

khellang commented Jun 26, 2018

Uh oh!

Tratcher commented Jun 26, 2018

Uh oh!

khellang commented Jun 27, 2018

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khellang commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

jkotalik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Tratcher Jun 26, 2018 •

edited

Loading

khellang commented Jun 27, 2018 •

edited

Loading