Add Brotli Compression to CoreFX #26229

ianhays · 2018-01-08T22:56:57Z

Adds source, tests, and packaging for Brotli compression and decompression to CoreFX.
Fulfills all requirements of the Brotli epic
Adds Brotli native code to clrcompression.dll and System.IO.Compression.Native.so
Brotli is in the System.IO.Compression namespace but in its own System.IO.Compression.Brotli assembly which is set as inbox for the next release

I've split up the code into five commits so reviewing the important stuff is easier. The commit with the source is the most interesting one.

resolves #25785
resolves dotnet/corefxlab#1728
resolves dotnet/corefxlab#1677
resolves https://github.com/dotnet/corefx/issues/24826
resolves dotnet/corefxlab#1958
resolves https://github.com/dotnet/corefx/issues/26089

ahsonkhan · 2018-01-08T23:13:16Z

src/System.IO.Compression.Brotli/tests/BrotliEncoderTests.cs

+        }
+
+        [Theory]
+        [OuterLoop("Full set of UncompressedTestFiles takes around 15s to run")]


Can you please trigger the Outerloop CI job as part of this PR so we can run these tests once now?

ahsonkhan · 2018-01-08T23:13:26Z

src/System.IO.Compression.Brotli/tests/BrotliEncoderTests.cs

+                var decompressedSpan = new Span<byte>(deompressed);
+
+                int totalWrittenThisIteration = 0;
+                var compres = encoder.Compress(uncompressedSpan, compressedSpan, out int bytesConsumed, out int bytesWritten, isFinalBlock: false);


nit: spelling compres

ahsonkhan · 2018-01-08T23:16:03Z

src/Common/tests/System/IO/Compression/CompressionStreamUnitTestBase.cs


            using (var writeStream = new ManualSyncMemoryStream(false))
            using (var compressor = CreateStream(writeStream, CompressionMode.Compress))
            {
                Task task = null;
                try
                {
+                    // Write needs to be bigger enough to trigger a write to the underlying base stream so the WriteAsync call doesn't immediately complete.


nit: Write needs to be big enough... here and elsewhere

joshfree · 2018-01-09T00:11:46Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliEncoder.cs

+                        }
+                        bytesConsumed += source.Length - (int)availableInput;
+                        bytesWritten += destination.Length - (int)availableOutput;
+                        if ((int)availableOutput == destination.Length) // no bytes written


Is there a scenario where no bytes are written and we're not in the done state (i.e., it's not okay to fall out of this if-check) If not, can you add and error case / assert here?

Not having any bytes written is a possible in more scenarios than the "Done" case (e.g. DestinationTooSmall which is caught by the next while iteration), but there's not anything special we have to do here unless the checks on line 154 are also passed, ensuring that we are "Done". We could add a check for availableOutput > 0 here instead of relying upon the while to fail, but it's probably not worth the code lines.

Really, line 151 and 154 should be one long if statement.

nit: why didn't you write it as one if-statement then?

jkotas · 2018-01-09T15:14:58Z

src/Common/src/Microsoft/Win32/SafeHandles/SafeBrotliHandle.cs

+
+        protected override bool ReleaseHandle()
+        {
+            if (handle != IntPtr.Zero)


ReleaseHandle is only ever call when the handle is non-null. You do not need this check.

From https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle.releasehandle?view=netcore-2.0: guaranteed to be called only once and only if the handle is valid

jkotas · 2018-01-09T15:15:33Z

src/Common/src/Microsoft/Win32/SafeHandles/SafeBrotliHandle.cs

+            if (handle != IntPtr.Zero)
+            {
+                Interop.Brotli.BrotliEncoderDestroyInstance(handle);
+                SetHandle(IntPtr.Zero);


You do not need to call SetHandle yourself. The caller does it for you.

You do not need to call SetHandle yourself. The caller does it for you.

You mean the runtime caller of this? I'm not seeing that. This:

using System; using System.Runtime.InteropServices; class Program { static void Main() { var msf = new MySafeHandle((IntPtr)12345); Console.WriteLine(msf.DangerousGetHandle()); msf.Dispose(); Console.WriteLine(msf.DangerousGetHandle()); } } class MySafeHandle : SafeHandle { public MySafeHandle(IntPtr handle) : base(IntPtr.Zero, ownsHandle: true) { SetHandle(handle); } public override bool IsInvalid => handle == IntPtr.Zero; protected override bool ReleaseHandle() { Console.WriteLine("Invoked"); return true; } }

prints:

12345 Invoked 12345

rather than:

12345 Invoked 0

(That said, I don't see a need to set the handle to zero here.)

You are right - the runtime caller will not do it for you. It just not necessary to set the handle to zero here.

It just not necessary to set the handle to zero here.

Yup, agreed.

jkotas · 2018-01-09T15:16:22Z

src/Common/src/Microsoft/Win32/SafeHandles/SafeBrotliHandle.cs

+
+        protected override bool ReleaseHandle()
+        {
+            if (handle != IntPtr.Zero)


jkotas · 2018-01-09T15:19:23Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliDecoder.cs

+        internal void InitializeDecoder()
+        {
+            _state = Interop.Brotli.BrotliDecoderCreateInstance(IntPtr.Zero, IntPtr.Zero, IntPtr.Zero);
+            if (_state == null || _state.IsInvalid || _state.IsClosed)


You will never get null _state here, and IsClosed check is redundant.

This can be just if (_state.IsInvalid).

jkotas · 2018-01-09T15:22:31Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliDecoder.cs

+        public void Dispose()
+        {
+            _disposed = true;
+            if (_state == null || _state.IsInvalid || _state.IsClosed)


This can be just:

if (_state != null) _state.Dispose();

Or just _state?.Dispose();

KrzysztofCwalina · 2018-01-09T16:50:28Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+            _mode = mode;
+            _stream = stream;
+            _leaveOpen = leaveOpen;
+            _buffer = new byte[DefaultInternalBufferSize];


Could we borrow this from the array pool? i.e. is it safe; do we ever hand the internal buffer to user code?

stephentoub · 2018-01-09T18:35:51Z

src/System.IO.Compression.Brotli/src/Interop/Interop.Brotli.Decoder.cs

@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.


Any reason this file isn't in src\Common\Interop, etc.?

Since they're interop-ing into our own binaries it felt weird to put them into src\Common\Interop. We have the zlib interop declarations under System.IO.Compression and not in Common so I was also following that pattern, though I don't have a great reason for not putting them into common instead.

I think it is fine to not have these under src\Common.

It would be nice to cleanup the few bits from these bits that are under Common for consistency:

src\Common\src\Interop\Unix\Interop.Libraries.cs(15): internal const string CompressionNative = "System.IO.Compression.Native";
src\Common\src\Interop\Windows\Interop.Libraries.cs(33): internal const string Zlib = "clrcompression.dll";

It would be nice to cleanup the few bits from these bits that are under Common for consistency:

Yup! I'm changing both of these to CompressionNative so I can get rid of the local "library.{os}.cs" files that I have.

stephentoub · 2018-01-09T18:37:45Z

src/System.IO.Compression.Brotli/src/Interop/Interop.Brotli.Decoder.cs

+{
+    internal static partial class Brotli
+    {
+        internal const string LibNameDecoder = Library.BrotliDecoder;


Nit: why is this const needed? It's just to be able to write "LibNameDecoder" rather than "Library.BrotliDecoder" in each DllImport? We don't do that elsewhere.

(And if we do keep it, why internal rather than private?)

stephentoub · 2018-01-09T18:38:29Z

src/System.IO.Compression.Brotli/src/Interop/Library.OSX.cs

+internal static partial class Library
+{
+    internal const string BrotliEncoder = "System.IO.Compression.Native.so";
+    internal const string BrotliDecoder = "System.IO.Compression.Native.so";


It's .so on macOS as well, not .dylib? If it's the same, why not just use a single .Unix.cs file?

stephentoub · 2018-01-09T18:40:11Z

src/System.IO.Compression.Brotli/src/Resources/Strings.resx

+    <value>Can not access a closed Encoder.</value>
+  </data>  
+  <data name="BrotliEncoder_Quality" xml:space="preserve">
+    <value>Provided BrotliEncoder Quality of ({0}) is not between the minimum value of {1} and the maximum value of {2}</value>


How come {0} is in parens but {1} and {2} are not?

stephentoub · 2018-01-09T18:40:52Z

src/System.IO.Compression.Brotli/src/Resources/Strings.resx

+  </data>  
+  <!-- BrotliStream.Compress -->
+  <data name="BrotliStream_Compress_UnsupportedOperation" xml:space="preserve">
+    <value>Cannot perform Read operations on a BrotliStream constructed with CompressionMode.Compress.</value>


Some of the strings use the two-word "can not" while others use the single-word "cannot"

stephentoub · 2018-01-09T18:43:55Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+
+namespace System.IO.Compression
+{
+    public partial class BrotliStream : Stream


Did we consider sealing it?

stephentoub · 2018-01-09T18:46:38Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+                }
+
+                if (disposing && !_leaveOpen)
+                    _stream?.Dispose();


We shouldn't need to check disposing and _stream for null twice and could instead do:

if (disposing && _stream != null) { if (_mode == CompressionMode.Compress) { WriteCore(...); } if (!_leaveOpen) { _stream.Dispose(); } }

stephentoub · 2018-01-09T18:47:52Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+            {
+                _stream = null;
+                _encoder.Dispose();
+                _decoder.Dispose();


We call Dispose on _encoder and _decoder even if disposing is false?

Ah, they're structs, ok.

stephentoub · 2018-01-09T18:49:04Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+        }
+
+        public Stream BaseStream => _stream;
+        public override bool CanRead => _mode == CompressionMode.Compress ? false : (_stream == null ? false : _stream.CanRead);


This seems like a strange way of writing it. Would it not be better as:

public override bool CanRead => _mode == CompressionMode.Decompress && _stream != null && _stream.CanRead;

?
Similar for CanWrite below.

stephentoub · 2018-01-09T18:50:50Z

src/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs

+            Debug.Assert(oldValue == 1, $"Expected {nameof(_activeAsyncOperation)} to be 1, got {oldValue}");
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]


I believe this attribute isn't needed anymore in this situation and in fact can make things worse.

Out of curiosity, how can it make things worse?

Without the NoInlining attribute, the JIT will try to inline the method, observes that it is not a good inlining candidate. The JIT will still take into account what it learned about the method while trying to inline it. It will know that the method always throws and never returns, and so it will move call to it to the cold section of the code, out of the hot path.

jkotas · 2018-01-09T23:50:27Z

src/System.IO.Compression.Brotli/src/System.IO.Compression.Brotli.csproj

+    </Compile>
+  </ItemGroup>
+  <!-- Windows specific files -->
+  <ItemGroup Condition=" '$(TargetsWindows)' == 'true' AND '$(TargetGroup)' != 'netfx'">


None of the configuration files list netfx. Why do you need the netfx conditions here?

We don't. I added this in attempt to address a build failure but it was actually due to something entirely unrelated. I removed the line and am iterating to resolve the failure now.

There are more places in the file that have the unnecessary netfx conditions.

ianhays · 2018-01-11T18:51:08Z

@jkotas @KrzysztofCwalina @stephentoub @ahsonkhan I responded to all feedback and all tests are passing except some unrelated System.Net failures.

Does anyone have any other concerns before I merge?

jkotas · 2018-01-11T20:07:11Z

Does anyone have any other concerns before I merge?

I am not able to look at this and comment on it effectively because of the size of the delta and together with comments is huge, and the browser is not cooperating. If you want to me to take an another look, submit & commit the verbatim sources copied from Brottli upstream in separate PR and leave this one to just the changes you made.

Because of some requirements for crc functions (log and exit), I had to open up the Windows AOT release native clrcompression builds to import from the crt api sets. My understanding is that this is fine as we are now packaging the crt api sets alongside microsoft.netcore.runtime.coreclr so we don't have to worry about them missing on the target machine. Similarly, the Unix native build had to change slightly as the current build makes use of some stringent checking on the target code which makes sense since they're currently just shims. I moved the failing checks to only be imposed on the still-shim native builds and ignore System.IO.Compression which is now filled with Brotli code.

ianhays · 2018-01-11T20:17:52Z

I am not able to look at this and comment on it effectively because of the size of the delta and together with comments is huge, and the browser is not cooperating. If you want to me to take an another look, submit & commit the verbatim sources copied from Brottli upstream in separate PR and leave this one to just the changes you made.

Done. I separated the google sources to a separate PR, merged them, and rebased this one on top. This PR is now only my code.

jkotas · 2018-01-12T00:13:01Z

src/Native/Unix/CMakeLists.txt

+
+add_compile_options(-Weverything)
+add_compile_options(-Wno-c++98-compat-pedantic)
+add_compile_options(-Werror)


We should keep warnings as errors by default for everything so that they do not creep up. It is annoying to look at the warnings during builds.

jkotas · 2018-01-12T00:22:55Z

src/Native/Unix/CMakeLists.txt

@@ -247,6 +240,14 @@ endfunction()
 include(configure.cmake)

 add_subdirectory(System.IO.Compression.Native)
+
+add_compile_options(-Weverything)


Enabling/disabling different warnings by changing the order here does not scale. Can we disable the specific warnings in System.IO.Compression.Native/CMakeLists.txt ?

I'm not aware of a way to remove a compile option once it's already been added. We could add them to each of the other library CmakeLists, but that scales worse than the ordering change.

CMake is pretty flexible and there's probably a way of getting all compile flags, parsing them for what you want to remove, then creating a new list without that item that is library specific. That would be more convoluted than just sticking the compression library add above the compile options though, and require more far effort to make sure it's exactly correct.

jkotas · 2018-01-12T00:25:25Z

src/Native/Windows/CMakeLists.txt

@@ -97,8 +94,6 @@ endif()

 if (__LinkCustomCRT STREQUAL "true" AND $ENV{__BuildArch} STREQUAL "arm" )
    set(__LinkArgs "${__LinkArgs} /DEFAULTLIB:ole32.lib")
-elseif(__LinkCustomCRT STREQUAL "true" AND NOT $ENV{__BuildArch} STREQUAL "arm" )
-    set(__LinkArgs "${__LinkArgs} /NODEFAULTLIB")
 endif()



The _tools_BinscopeFilter_packages_Newtonsoft.Json.9.0.1_lib_net45_Newtonsoft.Json.txt binary file is still here.

It shows on my github as being removed, which I'd like to do even though it isn't technically related to this PR

jkotas · 2018-01-12T00:33:16Z

src/Native/Windows/CMakeLists.txt

@@ -97,8 +94,6 @@ endif()

 if (__LinkCustomCRT STREQUAL "true" AND $ENV{__BuildArch} STREQUAL "arm" )
    set(__LinkArgs "${__LinkArgs} /DEFAULTLIB:ole32.lib")
-elseif(__LinkCustomCRT STREQUAL "true" AND NOT $ENV{__BuildArch} STREQUAL "arm" )


Why is this not needed anymore?

TL;DR: Now that we're packaging api sets in the runtime package, we don't need to link a custom crt here anymore

The "__LinkCustomCRT" property was added to the AppX build so we could build a clrcompression without dependencies on the CRT api sets. This was specifically to accommodate machines without the CRT available that were using AOT clrcompression builds. The idea was to use this as a stop-gap solution until Reverse Forwarders came in... RS2 I think it was? However, we appear to now be packaging the necessary crt api set dlls in the runtime package anyways, so we can just use those and not bother with the custom crt code.

The reason this actual line was removed was because this old custom crt of course doesn't provide all the functions that I needed for Brotli, so I needed to open up the default libs. Really I can remove all of the "__LinkCustomCRT" stuff, but I wanted to keep the delta here small.

There was some contention in the 1.0 timeframe around clrcompression and other native binaries requiring the target machine to have the crt api sets available, so that was the reasoning for this particular workaround (which was actually ripped from one of our netfx builds). If I remember correctly there was an ownership issue with statically linking what we needed because it put onto us the onus of servicing those dependencies. It appears that the long-term resolution for this in coreclr was to just package the api sets for the crt in the runtime package so we can dynamically link with abandon, but I've been off the radar on this for a while and missed the discussion on that. I imagine the cost of servicing/ownership was far lower than the cost of working around missing API sets on the target machine.

Though now that I think of it, since we're not including brotli for Uap right now, we don't need to change this as a part of this PR.

I can remove all of the "__LinkCustomCRT" stuff

Yes, I think that would be better than deleting just part of it.

jkotas · 2018-01-12T00:36:21Z

src/Native/Windows/CMakeLists.txt

@@ -51,9 +51,6 @@ add_compile_options(/Zl)          # enable debugging information
 add_compile_options(/wd4960 /wd4961 /wd4603 /wd4627 /wd4838 /wd4456 /wd4457 /wd4458 /wd4459 /wd4091 /we4640)
 add_compile_options(/ZH:SHA_256) # use SHA256 for generating hashes of compiler processed source files.

-if ($ENV{__BuildArch} STREQUAL "x86")
-    add_compile_options(/Gz)


There are potentially both correctness (calling convention mismatch) and performance (less efficient calling convention) implications from removing this switch on the existing clrcompression.dll. Have you verified that it is really safe to remove it?

I have, yes. The Brotli code isn't written to work with /Gz but the existing clrcompression is more flexible and works with the default calling convention.

The Brotli code isn't written to work with /Gz

Where is it not written to work with /Gz?

I'm not sure the exact position. Somewhere in the decompress routine via BrotliDecoderDecompressStream.

When we had /Gz in the build I was getting AVs out of that function on x86. I compared the output of our cmake build with the one at google/brotli, went through the differences, and determined /Gz to be the only thing that could cause the AVs , Removing it resolved the AVs and didn't affect the zlib part of clrcompression. I did some more searching around that suggested the underlying problem was likely brotli functions being written to rely upon a different calling convention.

Does that sound like a reasonable approach or is the removal of /Gz a bigger problem than I thought?

I do not see anything in the implementation that would depend on a particular x86 calling convention. It looks all plain vanilla portable C.

I think it would be useful to understand this better. E.g. there can be a stack overrun that leads to crash with /Gz, but leads to security hole without /Gz.

And yeah, looks like a stack overflow with /Gz

Have you also removed CDecl calling conventions from Interop.Brotli.* files and just let is use the default?

I have not, but their removal allows us to compile the native binary with /Gz without the failures. I pushed an update to do that instead.

So when I have /Gz enabled, clrcompression is compiled using the _stdcall convention. This was causing errors because my Interop files specified __cdecl as the convention because they were originally built around the official Brotli dlls (which use the default compiler setting), not my clrcompression dll.

Removing /Gz solved the errors because it made the calling convention of clrcompression match that of the official brotli dlls from which I built my interop code, in addition to the calling convention specified in the interop files. However, we can leave /Gz and resolve the errors by instead only changing the calling convention specified in the interop files to the default which is CallingConvention.WinAPI which uses the default platform calling convention e.g. on Windows the default is StdCall

You do not need to specify the calling convention at all, and it will then use the platform default (it is equivalent to WinAPI).

I would keep building it with /Gz and not specify the calling convention in DllImports (ie use the default). It is what we do in majority of other places.

Ok, I see that you have done exactly that.

jkotas · 2018-01-12T00:37:36Z

src/System.IO.Compression.Brotli/ref/System.IO.Compression.Brotli.csproj

+    <ProjectReference Include="..\..\System.Text.Encoding\ref\System.Text.Encoding.csproj" />
+    <ProjectReference Include="..\..\System.Threading.Tasks\ref\System.Threading.Tasks.csproj" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == 'netfx'">


Not necessary - this project does not have netfx configuration.

jkotas · 2018-01-12T00:38:45Z

src/System.IO.Compression.Brotli/src/System.IO.Compression.Brotli.csproj

+    <Reference Include="System.Threading" />
+    <Reference Include="System.Threading.Tasks" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == 'netfx'">


Not necessary?

jkotas · 2018-01-12T00:43:55Z

src/Native/Windows/clrcompression/CMakeLists.txt

@@ -40,6 +40,32 @@ elseif($ENV{__BuildArch} STREQUAL arm OR $ENV{__BuildArch} STREQUAL arm64)
    )
 endif()

+set (NATIVECOMPRESSION_SOURCES


This will build for UWP, but we are not shipping it on UWP. Omit it for UWP?

We can exclude these for "__AppContainer" builds but we don't have a notion of TargetGroup in the native build.

Sounds good to me.

jkotas · 2018-01-12T00:45:49Z

src/System.Data.Odbc/tests/System.Data.Odbc.Tests.csproj

@@ -3,6 +3,7 @@
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
    <ProjectGuid>{F3E72F35-0351-4D67-2209-725BCAD807BA}</ProjectGuid>
+    <DefineConstants Condition="'$(TargetsWindows)' == 'true'">$(DefineConstants);TARGETSWINDOWS</DefineConstants>


Many other projects are using TargetsWindows for the define, and I think it is also what the coding conventions tells you to use.

jkotas · 2018-01-12T00:47:42Z

src/System.Data.Odbc/tests/Helpers.cs

-            PlatformDetection.IsWindows ?
-                !PlatformDetection.IsWindowsNanoServer && (!PlatformDetection.IsWindowsServerCore || Environment.Is64BitProcess) :
+        private static bool CheckOdbcIsAvailable() =>
+#if TARGETSWINDOWS


What was wrong with this before? It would be useful to submit the ODBC fix separately since it has not much to do with the rest.

The issue was that this line was being included in the windows test build which meant we needed to also include the linux interop files in the windows build. That doesn't really make a ton of sense (and it led to some conflicts) so I made it so the unix interop call is only included in the unix build.

jkotas · 2018-01-13T01:32:44Z

src/Native/Windows/clrcompression/CMakeLists.txt

+if($ENV{__appContainer} STREQUAL true)
+    set (NATIVECOMPRESSION_SOURCES
+        ${NATIVECOMPRESSION_SOURCES}
+        clrcompression.AppX.def


https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/project-guidelines.md#code-file-naming-conventions says to use Uap for Uap specific stuff.

jkotas · 2018-01-13T01:34:45Z

src/Native/Windows/CMakeLists.txt

@@ -52,7 +52,7 @@ add_compile_options(/wd4960 /wd4961 /wd4603 /wd4627 /wd4838 /wd4456 /wd4457 /wd4
 add_compile_options(/ZH:SHA_256) # use SHA256 for generating hashes of compiler processed source files.

 if ($ENV{__BuildArch} STREQUAL "x86")
-    add_compile_options(/Gz)
+     add_compile_options(/Gz)


This whitespace change does not look like an improvement.

jkotas · 2018-01-13T01:36:05Z

src/System.IO.Compression.Brotli/ref/System.IO.Compression.Brotli.cs

+
+namespace System.IO.Compression
+{
+    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]


Do not need Sequential attributes in ref files. Wes just cleaned them up from everywhere.

jkotas · 2018-01-13T01:37:55Z

src/System.IO.Compression.Brotli/src/PinvokeAnalyzerExceptionList.analyzerdata

@@ -0,0 +1,23 @@
+<!-- Not a bug. clrcompression.dll is a native components that ship as part of the package. -->


Why do we need just the Brotli stuff here? What makes the analyzer to shut up for the existing ZLib stuff? Should both be on the same plan?

The Analyzer only cares about the exports that are actually used in the library. System.IO.Compression has its own copy of this file that only lists the zlib exports.

We could technically make it common and list all of the exports, but I preferred the separation and explicitness of each having their own file.

Thanks for the explanation. Separate files make sense.

jkotas · 2018-01-14T00:54:16Z

src/System.IO.Compression.Brotli/src/System.IO.Compression.Brotli.csproj

+  <ItemGroup>
+    <Reference Include="System.Buffers" />
+    <Reference Include="System.Collections" />
+    <Reference Include="System.Diagnostics.Contracts" />


Reference Include="System.Diagnostics.Contract should not be needed.

MattGal · 2018-01-16T22:11:12Z

@dotnet-bot test Linux x64 Release Build please
@dotnet-bot test OSX x64 Debug Build please
@dotnet-bot test UWP CoreCLR x64 Debug Build please
@dotnet-bot test Windows x64 Debug Build please
@dotnet-bot test Windows x86 Release Build please

ianhays · 2018-01-16T22:56:39Z

@dotnet-bot help

dotnet-bot · 2018-01-16T22:56:43Z

Welcome to the dotnet/corefx Repository

The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR

The following commands are valid for all PRs and repositories.

Click to expand

Comment Phrase	Action
@dotnet-bot test this please	Re-run all legs. Use sparingly
@dotnet-bot test ci please	Generates (but does not run) jobs based on changes to the groovy job definitions in this branch
@dotnet-bot help	Print this help message

The following jobs are launched by default for each PR against dotnet/corefx:master.

Click to expand

Comment Phrase	Job Launched
@dotnet-bot test Alpine.3.6 x64 Debug Build	Alpine.3.6 x64 Debug Build
@dotnet-bot test Linux x64 Release Build	Linux x64 Release Build
@dotnet-bot test OSX x64 Debug Build	OSX x64 Debug Build
@dotnet-bot test Packaging All Configurations x64 Debug Build	Packaging All Configurations x64 Debug Build
@dotnet-bot test Windows x64 Debug Build	Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build	Windows x86 Release Build
@dotnet-bot test NETFX x86 Release Build	NETFX x86 Release Build
@dotnet-bot test UWP CoreCLR x64 Debug Build	UWP CoreCLR x64 Debug Build
@dotnet-bot test UWP NETNative x86 Release Build	UWP NETNative x86 Release Build

The following optional jobs are available in PRs against dotnet/corefx:master.

Click to expand

Comment Phrase	Job Launched
@dotnet-bot test Outerloop Alpine.3.6 x64 Debug Build	Queues Outerloop Alpine.3.6 x64 Debug Build
@dotnet-bot test Alpine.3.6 x64 Release Build	Queues Alpine.3.6 x64 Release Build
@dotnet-bot test Outerloop Alpine.3.6 x64 Release Build	Queues Outerloop Alpine.3.6 x64 Release Build
@dotnet-bot test CentOS.6 x64 Debug Build	Queues CentOS.6 x64 Debug Build
@dotnet-bot test Outerloop CentOS.6 x64 Debug Build	Queues Outerloop CentOS.6 x64 Debug Build
@dotnet-bot test CentOS.6 x64 Release Build	Queues CentOS.6 x64 Release Build
@dotnet-bot test Outerloop CentOS.6 x64 Release Build	Queues Outerloop CentOS.6 x64 Release Build
@dotnet-bot test Linux x64 Debug Build	Queues Linux x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build	Queues Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Release Build	Queues Outerloop Linux x64 Release Build
@dotnet-bot test Outerloop OSX x64 Debug Build	Queues Outerloop OSX x64 Debug Build
@dotnet-bot test OSX x64 Release Build	Queues OSX x64 Release Build
@dotnet-bot test Outerloop OSX x64 Release Build	Queues Outerloop OSX x64 Release Build
@dotnet-bot test Outerloop Packaging All Configurations x64 Debug Build	Queues Outerloop Packaging All Configurations x64 Debug Build
@dotnet-bot test Packaging All Configurations x86 Debug Build	Queues Packaging All Configurations x86 Debug Build
@dotnet-bot test Outerloop Packaging All Configurations x86 Debug Build	Queues Outerloop Packaging All Configurations x86 Debug Build
@dotnet-bot test Packaging All Configurations x64 Release Build	Queues Packaging All Configurations x64 Release Build
@dotnet-bot test Outerloop Packaging All Configurations x64 Release Build	Queues Outerloop Packaging All Configurations x64 Release Build
@dotnet-bot test Packaging All Configurations x86 Release Build	Queues Packaging All Configurations x86 Release Build
@dotnet-bot test Outerloop Packaging All Configurations x86 Release Build	Queues Outerloop Packaging All Configurations x86 Release Build
@dotnet-bot test Outerloop Windows x64 Debug Build	Queues Outerloop Windows x64 Debug Build
@dotnet-bot test Windows x86 Debug Build	Queues Windows x86 Debug Build
@dotnet-bot test Outerloop Windows x86 Debug Build	Queues Outerloop Windows x86 Debug Build
@dotnet-bot test Windows x64 Release Build	Queues Windows x64 Release Build
@dotnet-bot test Outerloop Windows x64 Release Build	Queues Outerloop Windows x64 Release Build
@dotnet-bot test Outerloop Windows x86 Release Build	Queues Outerloop Windows x86 Release Build
@dotnet-bot test NETFX x64 Debug Build	Queues NETFX x64 Debug Build
@dotnet-bot test Outerloop NETFX x64 Debug Build	Queues Outerloop NETFX x64 Debug Build
@dotnet-bot test NETFX x86 Debug Build	Queues NETFX x86 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build	Queues Outerloop NETFX x86 Debug Build
@dotnet-bot test NETFX x64 Release Build	Queues NETFX x64 Release Build
@dotnet-bot test Outerloop NETFX x64 Release Build	Queues Outerloop NETFX x64 Release Build
@dotnet-bot test Outerloop NETFX x86 Release Build	Queues Outerloop NETFX x86 Release Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build	Queues Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test UWP CoreCLR x86 Debug Build	Queues UWP CoreCLR x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x86 Debug Build	Queues Outerloop UWP CoreCLR x86 Debug Build
@dotnet-bot test UWP CoreCLR x64 Release Build	Queues UWP CoreCLR x64 Release Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Release Build	Queues Outerloop UWP CoreCLR x64 Release Build
@dotnet-bot test UWP CoreCLR x86 Release Build	Queues UWP CoreCLR x86 Release Build
@dotnet-bot test Outerloop UWP CoreCLR x86 Release Build	Queues Outerloop UWP CoreCLR x86 Release Build
@dotnet-bot test UWP NETNative x64 Debug Build	Queues UWP NETNative x64 Debug Build
@dotnet-bot test Outerloop UWP NETNative x64 Debug Build	Queues Outerloop UWP NETNative x64 Debug Build
@dotnet-bot test UWP NETNative x86 Debug Build	Queues UWP NETNative x86 Debug Build
@dotnet-bot test Outerloop UWP NETNative x86 Debug Build	Queues Outerloop UWP NETNative x86 Debug Build
@dotnet-bot test UWP NETNative x64 Release Build	Queues UWP NETNative x64 Release Build
@dotnet-bot test Outerloop UWP NETNative x64 Release Build	Queues Outerloop UWP NETNative x64 Release Build
@dotnet-bot test Outerloop UWP NETNative x86 Release Build	Queues Outerloop UWP NETNative x86 Release Build

Have a nice day!

MattGal · 2018-01-17T05:45:54Z

@ianhays note we fixed a bug with CI earlier today that was hiding test failures from previous runs. I'd recommend doing appropriate diligence on the failures you see (for instance, seeing if the failures show up in boring, version-bump PRs...) and making a call whether to merge.

campersau · 2018-01-19T17:25:45Z

pkg/Microsoft.Private.PackageBaseline/packageIndex.json

@@ -1889,7 +1889,6 @@
    "System.IO.Compression.Brotli": {
      "InboxOn": {
        "netcoreapp2.1": "4.2.1.0",


I don't know how this file is parsed but normally trailing commas are not allowed in JSON.

Thanks, fixed.

ianhays · 2018-01-19T17:30:34Z

@dotnet-bot test Outerloop Packaging All Configurations x64 Debug Build
@dotnet-bot test Outerloop UWP NETNative x86 Debug Build
@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Release Build

Add Brotli Compression to CoreFX Commit migrated from dotnet/corefx@8f9cfa4

ianhays added api-approved API was approved in API review, it can be implemented area-System.IO.Compression labels Jan 8, 2018

ianhays added this to the 2.1.0 milestone Jan 8, 2018

ianhays self-assigned this Jan 8, 2018

ianhays requested review from GrabYourPitchforks, stephentoub, ahsonkhan, ViktorHofer and KrzysztofCwalina January 8, 2018 22:56

ahsonkhan reviewed Jan 8, 2018

View reviewed changes

joshfree reviewed Jan 9, 2018

View reviewed changes

karelz removed the api-approved API was approved in API review, it can be implemented label Jan 9, 2018

jkotas reviewed Jan 9, 2018

View reviewed changes

KrzysztofCwalina reviewed Jan 9, 2018

View reviewed changes

stephentoub reviewed Jan 9, 2018

View reviewed changes

jkotas reviewed Jan 9, 2018

View reviewed changes

Ian Hays added 4 commits January 11, 2018 12:15

Add tests for Brotli

6058279

Add packaging and build files for Brotli

1bc1fa8

Add Brotli source

a3b2e9b

jkotas suggested changes Jan 12, 2018

View reviewed changes

jkotas reviewed Jan 13, 2018

View reviewed changes

jkotas reviewed Jan 14, 2018

View reviewed changes

jkotas approved these changes Jan 16, 2018

View reviewed changes

dotnet deleted a comment from dotnet-bot Jan 16, 2018

ianhays mentioned this pull request Jan 17, 2018

Add MemoryExtensions Reverse API to get parity with array #26381

Merged

campersau reviewed Jan 19, 2018

View reviewed changes

Minor Brotli fixes

9ac1e71

ianhays merged commit 8f9cfa4 into dotnet:master Jan 19, 2018

jkotas mentioned this pull request Jun 11, 2018

Remove __LinkCustomCRT and crt.cpp #30256

Merged

ianhays deleted the brotli_b branch June 11, 2018 17:55

carlossanlop mentioned this pull request Oct 4, 2019

Document System.IO.Compression BrotliEncoder and BrotliStream dotnet/dotnet-api-docs#3298

Merged

carlossanlop mentioned this pull request Oct 19, 2019

Document System.IO.Compression.Brotli.BrotliDecoder dotnet/dotnet-api-docs#3379

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#26229 from ianhays/brotli_b

30cf604

Add Brotli Compression to CoreFX Commit migrated from dotnet/corefx@8f9cfa4

		@@ -0,0 +1,34 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -0,0 +1,23 @@
		<!-- Not a bug. clrcompression.dll is a native components that ship as part of the package. -->

Add Brotli Compression to CoreFX #26229

Add Brotli Compression to CoreFX #26229

Conversation

ianhays commented Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianhays commented Jan 11, 2018

jkotas commented Jan 11, 2018 • edited Loading

ianhays commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianhays commented Jan 8, 2018 •

edited

Loading

stephentoub Jan 9, 2018 •

edited

Loading

stephentoub Jan 9, 2018 •

edited

Loading

stephentoub Jan 9, 2018 •

edited

Loading

jkotas Jan 12, 2018 •

edited

Loading

jkotas Jan 9, 2018 •

edited

Loading

jkotas commented Jan 11, 2018 •

edited

Loading