System.IO.Compression: Make the ZLib BestCompression 6 #5458
Conversation
ianhays
added
tenet-performance
| @@ -72,7 +72,7 @@ public enum CompressionLevel : int | |||
| { | |||
| NoCompression = 0, | |||
| BestSpeed = 1, | |||
| BestCompression = 9, // Refer to "How to choose a compression level" above. | |||
| BestCompression = 6, // Refer to "How to choose a compression level" above. | |||
There was a problem hiding this comment.
I don't think this is the correct change. Zlib's "BestCompression" value is 9, so if we have an enum value here called "BestCompression", its value should match zlib's value.
There was a problem hiding this comment.
Instead of hard-coding our "Optimal" value to be "6" in zlib, I think our "Optimal" value should map to "-1"/"Z_DEFAULT_COMPRESSION" in zlib. See my comments on https://github.com/dotnet/corefx/issues/4475 for reasoning.
There was a problem hiding this comment.
Instead of hard-coding our "Optimal" value to be "6" in zlib, I think our "Optimal" value should map to "-1"/"Z_DEFAULT_COMPRESSION" in zlib.
I agree with that.
The changes will result in a pattern like so:
•CompressionLevel.Optimal => ZlibNative.CompressionLevel.DefaultCompression => Z_DEFAULT_COMPRESSION (-1)
•CompressionLevel.Fastest => ZlibNative.CompressionLevel.BestSpeed=> Z_BEST_SPEED (1)
•CompressionLevel.NoCompression => ZlibNative.CompressionLevel.NoCompression=> Z_NO_COMPRESSION (0)
Does that look right to you? ZlibNative.CompressionLevel.BestCompression will be removed since we won't be using it anywhere.
There was a problem hiding this comment.
Does that look right to you?
This mapping looks perfect to me.
ZlibNative.CompressionLevel.BestCompression will be removed since we won't be using it anywhere.
Yep - no reason to leave it in if it isn't used.
One additional thing to update is the native "PAL" layer that ensures our hard-coded values match the zlib we are compiling against:
There was a problem hiding this comment.
Thanks, I missed those. Updated.
EDIT: Updated with sizes as well |
|
@ianhays, what about size differences? |
|
Updated my above comment with sizes. |
CompressionLevel 6 is what we used to use and we changed it in #4589 because we didn't see any perf regressions in doing so. With more testing, it has become clear that moving from 6 to 9 severely hurt perf in some scenarios so it's being reverted to -1 which maps to 6.
Thanks. |
| /// ZLib definitions, which map to our public NoCompression, Fastest, and Optimal respectively. | ||
| /// <p><em>Optimal Compression:</em></p> | ||
| /// <p><code>ZLibNative.CompressionLevel compressionLevel = ZLibNative.CompressionLevel.BestCompression;</code> <br /> | ||
| /// <p><code>ZLibNative.CompressionLevel compressionLevel = ZLibNative.CompressionLevel.DefaultCompression;</code> <br /> |
There was a problem hiding this comment.
Are below comments around windowBits, etc., still correct?
|
LGTM. |
|
LGTM |
|
Thanks folks! |
System.IO.Compression: Make the ZLib BestCompression 6
System.IO.Compression: Make the ZLib BestCompression 6 Commit migrated from dotnet/corefx@78ad63e
CompressionLevel 6 is what we used to use and we changed it in #4589 because we didn't see any perf regressions in doing so. With more testing, it has become clear that moving from 6 to 9 severely hurt perf in some scenarios so it's being reverted back to 6.
resolves #4475
@stephentoub @eerhardt @bjjones
Perf results coming in subsequent comments.