-
Notifications
You must be signed in to change notification settings - Fork 272
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
Fixup Brotli compression tests #2567
base: main
Are you sure you want to change the base?
Conversation
As seen here, dotnet/runtime#73391 (comment), this test had a bug where in the work that was being done by the underlying test was not being correctly measured. This fix resolves that.
@@ -59,7 +59,7 @@ public void Compress() | |||
{ | |||
CompressedFile.CompressedDataStream.Position = 0; // all benchmarks invocation reuse the same stream, we set Postion to 0 to start at the beginning | |||
|
|||
var compressor = CreateStream(CompressedFile.CompressedDataStream, level); | |||
using var compressor = CreateStream(CompressedFile.CompressedDataStream, level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's less of an issue, but
performance/src/benchmarks/micro/libraries/System.IO.Compression/CompressionStreamPerfTestBase.cs
Line 71 in ccbeb4e
var compressor = CreateStream(CompressedFile.CompressedDataStream, CompressionMode.Decompress); |
@@ -13,7 +13,7 @@ public class Brotli : CompressionStreamPerfTestBase | |||
private const int Window = 22; | |||
|
|||
public override Stream CreateStream(Stream stream, CompressionMode mode) => new BrotliStream(stream, mode); | |||
public override Stream CreateStream(Stream stream, CompressionLevel level) => new BrotliStream(stream, level); | |||
public override Stream CreateStream(Stream stream, CompressionLevel level) => new BrotliStream(stream, level, leaveOpen: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same thing needs to be done in these:
performance/src/benchmarks/micro/libraries/System.IO.Compression/CompressionStreamPerfTestBase.cs
Lines 11 to 28 in ccbeb4e
public class Gzip : CompressionStreamPerfTestBase | |
{ | |
public override Stream CreateStream(Stream stream, CompressionMode mode) => new GZipStream(stream, mode); | |
public override Stream CreateStream(Stream stream, CompressionLevel level) => new GZipStream(stream, level); | |
} | |
public class Deflate : CompressionStreamPerfTestBase | |
{ | |
public override Stream CreateStream(Stream stream, CompressionMode mode) => new DeflateStream(stream, mode); | |
public override Stream CreateStream(Stream stream, CompressionLevel level) => new DeflateStream(stream, level); | |
} | |
#if NET6_0_OR_GREATER // API introduced in .NET 6 | |
public class ZLib : CompressionStreamPerfTestBase | |
{ | |
public override Stream CreateStream(Stream stream, CompressionMode mode) => new ZLibStream(stream, mode); | |
public override Stream CreateStream(Stream stream, CompressionLevel level) => new ZLibStream(stream, level); | |
} |
I think it'd also be cleaner if both CreateStream overloads always set leaveOpen: true, so it doesn't differ by which is invoked in the tests.
As seen here, dotnet/runtime#73391 (comment), this test had a bug where in
the work that was being done by the underlying test was not being correctly measured. This fix resolves that.