Change ZLib Optimal CompressionLevel on Unix

[eerhardt]

We currently have this comment and code in System.IO.Compression:

  // Note that ZLib currently exactly correspond to the optimal values.
  // However, we have determined the optimal values by intependent measurements across
  // a range of all possible ZLib parameters and over a set of different data.
  // We stress that by using explicitly the values obtained by the measurements rather than
  // ZLib defaults even if they happened to be the same.
  // For ZLib 1.2.3 we have (copied from ZLibNative.cs):
  // ZLibNative.CompressionLevel.DefaultCompression = 6;
  // ZLibNative.Deflate_DefaultWindowBits = -15;
  // ZLibNative.Deflate_DefaultMemLevel = 8;

  case CompressionLevel.Optimal:
      zlibCompressionLevel = (ZLibNative.CompressionLevel)6;
      windowBits = -15;
      memLevel = 8;
      strategy = ZLibNative.CompressionStrategy.DefaultStrategy;
      break;

Since we are now using the “current” ZLib version on Unix, and because we are planning on changing which version we use on Windows, I don’t think this is the best approach anymore. It was probably correct when we were hard-coding a version, but since we don’t know which version will actually be used, I think we should be letting the zlib library figure out which is the best compression level to use.

I have 2 separate options on how to address this:

1. On Unix, create a new shim method DeflateInit that only takes in the ZStream. The shim code will use the default values from the ZLib headers. Windows remains unchanged.
   eerhardt/corefx@c1be54d
2. Change both Windows and Unix code to always use the default compression level (-1) and let the zlib version pick.
   eerhardt/corefx@38883e3

@ianhays @stephentoub - Do you guys agree this should be fixed? If so, do you have a preferred option - or an even better option than the 2 I've listed?

[stephentoub]

I'm probably just missing a step as I've not looked through all that option-setting code in a while.

But zlib exposes a scale from fastest to best compression, with defined constants for the limits:

#define Z_NO_COMPRESSION         0
#define Z_BEST_SPEED             1
#define Z_BEST_COMPRESSION       9
#define Z_DEFAULT_COMPRESSION  (-1)

and in System.IO.Compression we have the CompressionLevel enum:

public enum CompressionLevel
{
    Optimal = 0,
    Fastest = 1,
    NoCompression = 2
}

Why wouldn't we just map:

• CompressionLevel.Optimal => Z_BEST_COMPRESSION
• CompressionLevel.Fastest => Z_BEST_SPEED
• CompressionLevel.NoCompression => Z_NO_COMPRESSION

?

Based on the documented meaning of the .NET enum values and the zlib constants, those would appear to be an exact match.

Or am I totally misunderstanding?

[ianhays]

@stephentoub looks right to me. Though CompressionLevel.Optimal should be set to Z_DEFAULT_COMPRESSION instead of Z_BEST_COMPRESSION.

[eerhardt]

I think you are missing the other comment on ZLibNative.CompressionLevel.

Here are the highlights:

        /// <code>CompressionLevel.DefaultCompression</code> = -1 requests a default compromise between speed and compression
        /// (currently equivalent to level 6).</p>
...
        ///   <li><code>CompressionLevel</code> values over 6 do not significantly improve the compression rate,
        ///       however such values require additional compression time.</li>
        ///   <li>Thus it is not recommended to use a compression level higher than 6, including the
        ///       value <code>CompressionLevel.BestCompression</code>.</li>
        ///   <li>The optimal compression performance (time/rate ratio) tends to occur at compression level 6
        ///       (<code>CompressionLevel.DefaultCompression</code>).</li>

What I am proposing is a slightly different map than what you have:

• CompressionLevel.Optimal => Z_DEFAULT_COMPRESSION
• CompressionLevel.Fastest => Z_BEST_SPEED
• CompressionLevel.NoCompression => Z_NO_COMPRESSION

[stephentoub]

Though CompressionLevel.Optimal should be set to Z_DEFAULT_COMPRESSION instead of Z_BEST_COMPRESSION

Why?

EDIT: I just saw @eerhardt's rsponse that answers my Why.

[stephentoub]

Here are the highlights

Where does that text come from originally, i.e. is it from zlib docs somewhere and copied into our .cs files, or was it written by a dev who worked on System.IO.Compression? I see no mention of "it is not recommended to use a compression level higher than 6" in the zlib docs... am I just missing it? Just seems strange that we'd provide an option that says "ask for the best compression possible" but then dilute that.

[stephentoub]

Regardless, I agree we should map the .NET enum values to the corresponding zlib values. Then it's just a matter of what we choose for "optimal compression".

[ianhays]

The msdn docs for Optimal do say The compression operation should be optimally compressed, even if the operation takes a longer time to complete. which directly conflicts with the comments on CompressionLevel stating that it isn't worth the extra compression time.

The comments for CompressionLevel may be outdated with regards to the usefulness of the higher compression level.

If we do set it to Z_BEST_COMPRESSION then we no longer have a good "middle-ground" compression level: we can only choose between no compression, fast compression, or best compression.

[eerhardt]

Where does that text come from, i.e. is it from zlib docs somewhere or was it written by a dev who worked on System.IO.Compression?

I assume it came from our internal devs, and not from zlib.

The furthest back I could trace it was to @tarekgh in this changeset:
764226 add Tarek Mahmoud Sayed 9/20/2012 6:00:22 PM $/DevDiv/OffCycle11/Redhawk/V2/ProjectN/FX/src/System.IO.Compression/System/IO/Compression/ZLibNative.cs Initial porting for System.IO.Compression This checkin enable DeflateStream class and use the managed library for deflat...

I'm sure it came from earlier, but I don't know where the earlier code was...

[eerhardt]

If we do set it to Z_BEST_COMPRESSION then we no longer have a good "middle-ground" compression level: we can only choose between no compression, fast compression, or best compression.

Does that imply that we need a new enum value?

[stephentoub]

Does that imply that we need a new enum value?

Maybe not. The docs also state that you get Optimal when you don't specify a level, but the enum value choice isn't to my knowledge actually exposed anywhere, so what this really means is you get the compromise by default between speed/compression. We could make it so that if you don't specify a level, you get Z_DEFAULT_COMPRESSION, and if you do specify a level, then you're doing so because you want to change it to either best speed, best compression, or no compression, and we honor that.

Just a suggestion. There may be gotchas I'm not considering.

[ianhays]

Does that imply that we need a new enum value?

That's what I was getting at, yeah. It's a bit odd to me that we don't currently have a "default" compression level that is a middle-ground between speed and best.

We could make it so that if you don't specify a level, you get Z_DEFAULT_COMPRESSION, and if you do specify a level, then you're doing so because you want to change it to either best speed, best compression, or no compression, and we honor that.

This is good with me, too. Since calling a function with CompressionLevel.Default would be the same as calling a function without a specified CompressionLevel, I can see us leaving out the CompressionLevel.Default definition. My gut reaction is that including Default in the enum is the better choice since it allows Default to be passed around more easily, but I don't have a good example where this would be useful.

[stephentoub]

My gut reaction is that including Default in the enum is the better choice

Yes, we would want to make it available eventually, I just meant to imply that we wouldn't need to block on it.

[eerhardt]

Since the enum is already defined like this:

 public enum CompressionLevel
 {
     Optimal = 0,
     Fastest = 1,
     NoCompression = 2
 } 

It would feel wrong to have either:
Default = -1
or
Default = 3.

I think it is good practice to have the "default" enum value be the same as default(T) would give you.

I think long term it would be better if we added a new value "BestCompression = 3", and changed the MSDN docs. IMO, the docs are and have been incorrect since the code has been using the value "6" since at least 2012.

[ianhays]

Instead of adding BestCompression and making it 3, why not just make Optimal=3?