perf: replace CFFI with a native C extension #76

tseaver · 2021-08-24T22:09:25Z

Toward #68.

Also, run unit tests during 'check' scripts. Drop installation of 'cffi' / 'pycparser'.

Explicily control building the extension, so that we can set the link-time and runtime library paths for libcrc32c.so.

Drop options for extension which were germane for only the local Linux build.

Explicily control building the extension, so that we can set the link-time and runtime library paths for libcrc32c.so.

Enables compiler optimizations.

Temporary hack while debugging Windows CI.

tseaver · 2021-08-25T16:16:29Z

BUILDING.md

+$ venv/bin/python setup.py build_ext \
+    --include-dirs=$(pwd)/usr/include \
+    --library-dirs=$(pwd)/usr/lib \
+    --rpath=$(pwd)/usr/lib


Note to reviewers: this change, and the equivalent ones in the scripts/ directory files below) is needed to ensure that we build with the expected local version of libcrc32c.so: pip doesn't provide a fine-enough-grained way to pass through these arguments to the bulld_ext command, so we just run it ourselves, and let pip use the already-build extension when making the wheel.

Thanks for this info. I was originally hoping to upgrade the build system to cibuildwheel because it has built-in cross-compiling support. If I understand cibuildwheel correctly it might not handle more fine-tuned manual steps like this very well, in which case we'll have to manually cross-compile as well. I'm still learning about this process, though.

I haven't worked with cibuildwheel at all. If it is run from a Github action, maybe we could configure the build_ext step to run before kicking it off?

tseaver · 2021-08-25T16:24:31Z

setup.py

+    package_dir={"": "src"},
+    ext_modules=ext_modules,
+    cmdclass={"build_ext": BuildExtWithDLL},
+)


Note to reviewers: rather than implicitly falling back to a pure-Python version (which is horribly slow, and almost certainly not what the user really wants), I chose not to trap / wrap the build_ext failures: it made debugging such failures on CI much simpler, and I think makes for a more pleasant UX.

We might want to consider doing and explicit build with the CRC32C_PURE_PYTHON envvar set, generating a none-any wheel. OTOH, that means that users might get that wheel unexpectedly.

This only impacts the build step but not the end user-facing fallback feature, right?

Right. The difference here is that the user has to make an explicit choice at build time to build / install the pure Python fallback: given how poor its performance is, I think it makes sense to have the user opt-in.

Once they've done that, then the runtime warning will be emitted when they first import the library.

tseaver · 2021-08-25T16:26:28Z

src/google_crc32c/cext.py

-    """
-    return google_crc32c._crc32c_cffi.lib.crc32c_value(chunk, len(chunk))
-
-


Note to reviewers: part of the win in this PR is avoiding this Python function call to get the chunk length computed: instead, the extension does that using PyArg_ParseTuple.

tseaver · 2021-08-25T16:28:03Z

tests/test___init__.py

@@ -178,32 +178,34 @@ def test_extend_w_multiple_chunks():
    crc = 0

    for chunk in iscsi_chunks(7):
-        crc = google_crc32c.extend(crc, chunk)
+        chunk_bytes = bytes(chunk)
+        crc = google_crc32c.extend(crc, chunk_bytes)


Note to reviewers: one consequence of moving to the C extension is that we no longer implicitly take a list-of-int as a substitute for bytes. Given that no real user is ever going to pass that, I don't think it matters.

tseaver · 2021-08-25T16:28:31Z

@andrewsg, @crwilcox I think this PR is ready for review.

crwilcox · 2021-08-25T16:10:44Z

scripts/windows/build.bat

@@ -39,7 +39,7 @@ py -%PYTHON_VERSION% -m pip install cmake

 git submodule update --init --recursive

-FOR %%V IN (32,64) DO (ddd


crwilcox · 2021-08-25T16:29:28Z

setup.py

-    except SystemExit:
-        # If installation fails, it is likely a compilation error with CFFI
-        # Try to install again.
-        warnings.warn(


Would it be good to continue serving this warning? I think there is a runtime one as well, so may not be crucial.

What I found was that trapping the SystemExit here caused CI to lose the error message from the build_ext step. I found that way more useful than the warning. Having the warning at runtime is a good belt-and-suspenders, though.

tseaver · 2021-08-25T18:09:33Z

Note to reviewers: FWIW, I tried first to have the library build generate a static library (libcrc32c.a / crc32c.lib). That worked find for the local linux build, but I couldn't make that work cleanly on CI. I think that would be a better DX (they wouldn't need to get libcrc32c.so / crc32c.dll installed somewhere on the library search path), so we could revisit if you think it is important.

andrewsg

Thanks a lot for your time and expertise here.

andrewsg · 2021-08-25T17:53:03Z

BUILDING.md

+$ venv/bin/python setup.py build_ext \
+    --include-dirs=$(pwd)/usr/include \
+    --library-dirs=$(pwd)/usr/lib \
+    --rpath=$(pwd)/usr/lib


Thanks for this info. I was originally hoping to upgrade the build system to cibuildwheel because it has built-in cross-compiling support. If I understand cibuildwheel correctly it might not handle more fine-tuned manual steps like this very well, in which case we'll have to manually cross-compile as well. I'm still learning about this process, though.

andrewsg · 2021-08-25T17:56:52Z

scripts/local-linux/build_libcrc32c.sh

@@ -42,6 +42,14 @@ ${VENV}/bin/cmake \
 # Install `libcrc32c` into CRC32C_INSTALL_PREFIX.
 make all install

+cd ${REPO_ROOT}
+
+${VENV}/bin/python setup.py build_ext \


For my own edification, why does this step and the next need to be moved into the build_libcrc32c.sh file due to the migration off of cffi?

pip wheel doesn't provide fine-grained enough support for passing the --include-dirs / --library-dirs / --rpath options down to the build_ext command: we want to ensure that the extension is built against the expected headers / libraries, and so we run the biuld_ext command explicitly to compile the extension before invoking pip wheel.

andrewsg · 2021-08-25T18:03:18Z

setup.py

+    package_dir={"": "src"},
+    ext_modules=ext_modules,
+    cmdclass={"build_ext": BuildExtWithDLL},
+)


This only impacts the build step but not the end user-facing fallback feature, right?

andrewsg · 2021-08-25T18:14:44Z

src/google_crc32c/cext.py

-    """
-    return google_crc32c._crc32c_cffi.lib.crc32c_value(chunk, len(chunk))
-
-


tseaver added 2 commits August 24, 2021 17:06

perf: replace CFFI w/ native C extension

d78e70a

Toward #68.

ci: update 'check' script to use native extension

f49d461

tseaver requested review from crwilcox and andrewsg August 24, 2021 22:09

tseaver requested a review from a team as a code owner August 24, 2021 22:09

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2021

tseaver added 2 commits August 24, 2021 18:25

ci: ensure we build static lib

d4467dd

Also, run unit tests during 'check' scripts. Drop installation of 'cffi' / 'pycparser'.

ci: revert building static lib under Windows

6f906b4

tseaver force-pushed the 68-perf-native-c-extension branch from c28a20c to 6f906b4 Compare August 24, 2021 22:32

tseaver added 13 commits August 25, 2021 08:12

ci: revert building static lib under manylinux

3f4dce9

ci: revert building static lib under local linux

85219c7

Explicily control building the extension, so that we can set the link-time and runtime library paths for libcrc32c.so.

build: require opt-in to build pure-Python version

b733045

Drop options for extension which were germane for only the local Linux build.

ci: revert building static lib on osx

8897297

Explicily control building the extension, so that we can set the link-time and runtime library paths for libcrc32c.so.

ci: manually build extension to ensure include path is set on Windows

00c9031

ci: build release version of libcrc32c

db32acf

Enables compiler optimizations.

chore: fix typo

0c83e16

ci: add manual lib path for windows build of extension

00fb5b8

ci: further tweak Windows build of C extension

84c7966

build: avoid swallowing error from buiding extension

0a45e7b

Temporary hack while debugging Windows CI.

ci: moar Windows tweakage

a6ae34f

ci: further Windows tweakage

3a37f0b

fix: return value is unsigned in C

ab4aa92

tseaver mentioned this pull request Aug 25, 2021

Investigate and close potential performance gap between other crc32c implementations? #68

Closed

fix: parse existing crc as unsigned long

7a3a640

tseaver commented Aug 25, 2021

View reviewed changes

crwilcox approved these changes Aug 25, 2021

View reviewed changes

andrewsg approved these changes Aug 25, 2021

View reviewed changes

tseaver merged commit b1bf461 into master Aug 25, 2021

tseaver deleted the 68-perf-native-c-extension branch August 25, 2021 18:33

rcash mentioned this pull request Aug 31, 2021

Newly released 1.1.3 cannot be installed using pip3 #82

Closed

mbrancato mentioned this pull request Aug 31, 2021

Cannot build 1.1.3 on Alpine #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: replace CFFI with a native C extension #76

perf: replace CFFI with a native C extension #76

tseaver commented Aug 24, 2021

tseaver Aug 25, 2021

andrewsg Aug 25, 2021

tseaver Aug 25, 2021

tseaver Aug 25, 2021

andrewsg Aug 25, 2021

tseaver Aug 25, 2021

tseaver Aug 25, 2021

andrewsg Aug 25, 2021

tseaver Aug 25, 2021

tseaver commented Aug 25, 2021

crwilcox Aug 25, 2021

crwilcox Aug 25, 2021 •

edited

tseaver Aug 25, 2021

tseaver commented Aug 25, 2021

andrewsg left a comment

andrewsg Aug 25, 2021

andrewsg Aug 25, 2021

tseaver Aug 25, 2021

andrewsg Aug 25, 2021

andrewsg Aug 25, 2021

		"""
		return google_crc32c._crc32c_cffi.lib.crc32c_value(chunk, len(chunk))

		@@ -39,7 +39,7 @@ py -%PYTHON_VERSION% -m pip install cmake

		git submodule update --init --recursive

		FOR %%V IN (32,64) DO (ddd

perf: replace CFFI with a native C extension #76

perf: replace CFFI with a native C extension #76

Conversation

tseaver commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tseaver commented Aug 25, 2021

Choose a reason for hiding this comment

crwilcox Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tseaver commented Aug 25, 2021

andrewsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crwilcox Aug 25, 2021 •

edited