License, Windows and Compatibility Upgrade (redux) by neurolabusc · Pull Request #19 · cloudflare/zlib

neurolabusc · 2020-02-14T03:24:15Z

This pull request responds to feedback to an earlier pull request.

It adds 3 benefits:

The GPL code for SIMD CRC has been replaced by BSD-license code from the Chrome browser. Performance is identical, as demonstrated in this minimal project. This addresses complications raised with GPL code.
At run-time, the software now detects if the host computer supports CLMUL instructions, using the accelerated CRC if supported. This prevents the software from executing illegal instructions on hardware that does not support CLMUL or where CLMUL has been disabled by a OEM BIOS or virtual machine. The code for detection is designed to be backward compatible with old compilers (e.g. gcc 4.8.4) that report to CMAKE that they support CLMUL instructions but do not provide the cpuid detection of CLMUL.
The CMAKE files have been updated to support modern Microsoft compilers allowing simpler distribution to those using the Windows operating system.

I would be excited to see this pull request included, as it can be called from the pigz make. This dramatically accelerates pigz, with the SIMD CRC directly accelerating the serial stage that is most impacted by Amdahl's law.

The first time the CRC is called it set the atomic int cpu_has_pclmul, which reports whether or not the clmul instructions are available. This ensures the program does not crash if compiled on a modern computer and executed on an old computer. I appreciate different people have different views on this, which I summarize below. I do not have a preference, the pull request I am providing is robust (tested on Ubuntu 14.04 as well as recent versions) and simple. The CRC SIMD code and CPUID is only tested for x86-64, but again this extends the current repository. Future developments my consider alternatives:

@Adenilson suggests pthread_once_t, though @mscastanho criticized this approach.
@mscastanho prefers ifunc but @Adenilson was concerned that the performance is not great and it is not available on all platforms.
@vkrasnov suggested attribute((constructor)) but @kornelski was not sure if that was the right approach.

Other comments

@Adenilson suggested SIMD CRC for ARM but I am unable to validate that platform.
The compiler -mpclmul flag is only applied to the crc32_simd unit. In theory, if this flag were applied globally, a compiler could optimize other sections of code to use these instructions, and the resulting code would fail if run on older (<2009) computers.

For most cases (where files are large) testing the CPU for CLMUL support on has negligible performance impact. One can conceive of use cases where an statically compiled executable is called many times to compress tiny files. If one is confident the resulting executable is going to be only run on modern (Westmere or later) CPUs, one can compile with the DSKIP_CPUID_CHECK compiler directive to skip this check for cmake the command is "cmake -DSKIP_CPUID_CHECK=ON ..".

Single threaded performance better than other zlib libraries
Multi-threaded preformance better than other zlib libraries

…ttps://github.com/neurolabusc/simd_crc

…rtium/ITK#416)

…ei/zlib)

…/zlib#25)

…on execution.

vkrasnov · 2020-02-25T00:42:20Z

configure

 #include <immintrin.h>
-void foo(void) {
-  _mm_clmulepi64_si128(_mm_set1_epi16(1), _mm_set1_epi16(2), 0);
+#include <wmmintrin.h>


this include is not required AFAIK

vkrasnov · 2020-02-25T00:43:06Z

crc32.c

+ #ifndef _MSC_VER
+  #include <cpuid.h>
+ #endif
+ //#include <stdio.h>


please remove

vkrasnov · 2020-02-25T00:43:20Z

crc32.c

+	#endif
+		if ((ecx & crc_bit_PCLMUL) != 0)
+			cpu_has_pclmul = 1;		
+		//printf("leaf=%d, eax=0x%x, ebx=0x%x, ecx=0x%x, edx=0x%x, cpu_has_pclmul %d\n", leaf, eax, ebx, ecx, edx, cpu_has_pclmul);


please remove

vkrasnov · 2020-02-25T00:44:00Z

crc32.c

+	#ifndef SKIP_CPUID_CHECK
+	if (!has_pclmul())
+      return crc32_generic(crc, buf, len);
+    #endif


Not aligned with the #ifndef above it

vkrasnov · 2020-02-25T00:45:06Z

deflate.c

 * allocation.
 */
-uint64_t ZEXPORT deflateBound(strm, sourceLen)
+uLong ZEXPORT deflateBound(strm, sourceLen)


why is this changed?

neurolabusc · 2020-02-25T16:44:51Z

@vkrasnov thanks for your careful review. I believe my latest commit includes all your suggestions.

vkrasnov · 2020-02-26T04:17:03Z

@neurolabusc thank you very much for the contribution!

Add a "compression context" to CompressionStream and DecompressionStream to make the tracking of the internal state explicit.

neurolabusc added 16 commits January 23, 2020 18:44

Replace GPL CRC with BSD CRC (zlib-ng/zlib-ng#42), for validation see h…

aa1deea

…ttps://github.com/neurolabusc/simd_crc

Westmere detection

1f5a420

Update configure for Westmere (InsightSoftwareConsortium/ITK#416)

2937f89

use cpu_has_pclmul() to autodetect CPU hardware (InsightSoftwareConso…

dd3e924

…rtium/ITK#416)

remove gpl code

b3064e0

Improve support for compiling using Windows (https://github.com/ningf…

4c77bc7

…ei/zlib)

Import ucm.cmake from https://github.com/ningfei/zlib

4bf55a2

crc32_simd as separate file (#18)

59c0c27

atomic and SKIP_CPUID_CHECK (#18)

dc28f91

Remove unused code

b73befc

Only allow compiler to use clmul instructions to crc_simd unit (intel…

df5c4fe

…/zlib#25)

Atomic does not compile on Ubuntu 14.04

c99aa75

Allow "cmake -DSKIP_CPUID_CHECK=ON .." to not check SIMD CRC support …

0324e35

…on execution.

Restore Windows compilation using "nmake -f win32\Makefile.msc"

99f7b58

zconf.h required for Windows nmake

f9254d5

support crc intrinsics for ARM CPUs

47171b6

vkrasnov reviewed Feb 25, 2020

View reviewed changes

crc32.c Outdated

#ifndef _MSC_VER

#include <cpuid.h>

#endif

//#include <stdio.h>

Copy link

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

vkrasnov reviewed Feb 25, 2020

View reviewed changes

crc32.c

#ifndef SKIP_CPUID_CHECK

if (!has_pclmul())

return crc32_generic(crc, buf, len);

#endif

Copy link

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not aligned with the #ifndef above it

vkrasnov reviewed Feb 25, 2020

View reviewed changes

Requested changes (#19)

e5248c1

kornelski approved these changes Feb 25, 2020

View reviewed changes

vkrasnov merged commit 372bcd1 into cloudflare:gcc.amd64 Feb 26, 2020

ningfei mentioned this pull request Nov 18, 2020

CMake update. #25

Merged

fhanau pushed a commit to fhanau/zlib that referenced this pull request Feb 27, 2023

Add the concept of a "compression context" (cloudflare#19)

85b3211

Add a "compression context" to CompressionStream and DecompressionStream to make the tracking of the internal state explicit.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

License, Windows and Compatibility Upgrade (redux)#19

License, Windows and Compatibility Upgrade (redux)#19
vkrasnov merged 17 commits intocloudflare:gcc.amd64from
rordenlab:gcc.amd64

neurolabusc commented Feb 14, 2020

Uh oh!

vkrasnov Feb 25, 2020

Uh oh!

vkrasnov Feb 25, 2020

Uh oh!

vkrasnov Feb 25, 2020

Uh oh!

vkrasnov Feb 25, 2020

Uh oh!

vkrasnov Feb 25, 2020

Uh oh!

neurolabusc commented Feb 25, 2020

Uh oh!

vkrasnov commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

neurolabusc commented Feb 14, 2020

Uh oh!

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

vkrasnov Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

neurolabusc commented Feb 25, 2020

Uh oh!

vkrasnov commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants