Conversation
From the PR you linked:
Is this no longer needed? |
FYI: @ahsonkhan |
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.
LGTM. @ianhays @ahsonkhan can you please complete some loop runs / stress testing on this change?
@@ -1,5 +1,5 @@ | |||
/* deflate.c -- compress data using the deflation algorithm | |||
* Copyright (C) 1995-2013 Jean-loup Gailly and Mark Adler | |||
* Copyright (C) 1995-2017 Jean-loup Gailly and Mark Adler |
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.
I'm curious, what causes some of these copyright dates to differ, e.g. https://github.com/dotnet/corefx/pull/32732/files#diff-d1964061700ad02918d1ccd83e7e4a86R2 being updated to 2016 and this being updated to 2017?
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 copyright headers (including the dates) were retained from the upstream ZLib for existing files that we modified.
@Wraith2 The deflate_fast strategy is the current default for CompressionLevel Fastest in the latest version of ZLib-Intel. I have updated the PR with performance details for various compression levels. |
zlib/compress.c | ||
zlib/inffast.c | ||
zlib/inflate.c | ||
zlib/inftrees.c |
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.
Why were these previously zlib when the rest were zlib-intel? Is it that these files weren't previously specialized in the Intel implementation, and now they are?
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.
Yes, we added new optimizations for decompression in the latest ZLib-Intel. This was not available previously.
Is this the update that was referenced as being expected by middle of June? If yes, it's fine that it's delayed, but I'd like to understand why the four month delay to understand if there's something to be on the look out for as a result. Was there some kind of risk that caused the slip, some failures that required mitigation and lots of extra testing, etc.? |
@stephentoub Yes it is referencing this update. The initial version of ZLib-Intel with optimizations for upstream ZLib v1.2.11 was available in the June time-frame. Some of the delay since June was from evaluating additional optimizations and testing. We checked to ensure our schedule aligned with the .NET Core 3.0 release cycle. |
@stephentoub the delay is mostly our fault due to various leaves / time off on our end; we asked @vkvenkat to take their time and delay until more people were back on our side to help review and test the changes. |
Ok, thanks. I mainly just wanted to know if there were any significant issues we should be aware of, whether we should be looking at beefing up testing in any particular areas, etc. |
@vkvenkat do you have any performance numbers for non-Intel hardware? |
@danmosemsft We don't have any performance numbers for non-Intel hardware. |
if (x86_cpu_has_sse42) | ||
return insert_string_sse(s, str); | ||
#ifdef USE_SSE_SLIDE | ||
if (x86_cpu_has_sse2) |
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.
Do we need this, since CoreCLR requires SSE2 for both x86 and x64? Is this to support other runtimes using CoreFX that might not have the same requirement?
@vkvenkat could you resolve the merge conflict and then we're good to merge this PR to master. Thank you |
@joshfree Done. |
Thanks, @vkvenkat. |
This change introduced the following warnings in master:
|
@ViktorHofer, can you open an issue for this? @vkvenkat, can you please address this with a follow-up PR? We really want our build to be warning and error free. At the moment there are a few other warnings due to an issue with one of the tools used in the build, but hopefully those will also be addressed soon. |
@stephentoub Sure, I will make a follow-up PR to fix this. |
Thanks, @vkvenkat and @ViktorHofer. |
Warnings fixed by PR #33357 |
Thanks @vkvenkat |
Commit migrated from dotnet/corefx@c89e274
This PR fixes #28013 and updates ZLib-Intel to v1.2.11 as available in https://github.com/jtkukunas/zlib.
The below performance numbers were captured with the Canterbury Corpus workload on a machine with an Intel® Core™ i7-8700K CPU on Windows.
When compared to ZLib-vanilla v1.2.11:
In addition, this release has security and bug fixes from the latest upstream ZLib.
Here is a detailed performance comparison for ZLib-Intel v1.2.11 (https://github.com/jtkukunas/zlib) vs ZLib v1.2.11 (https://github.com/madler/zlib) by streams & compression levels:
Here are the compression ratios for DeflateStream with the Fastest compression level:
Here are the compression ratios for DeflateStream with the Optimal compression level:
Similar to PR #5674, there are small increases in the compression ratio for Optimal.
@ianhays @joshfree @atsushikan PTAL