diff --git a/packages/google-crc32c/src/google_crc32c/__init__.py b/packages/google-crc32c/src/google_crc32c/__init__.py index 0acf5476ba2f..f2452c7a3f6b 100644 --- a/packages/google-crc32c/src/google_crc32c/__init__.py +++ b/packages/google-crc32c/src/google_crc32c/__init__.py @@ -34,7 +34,14 @@ extend = impl.extend value = impl.value +GIL_RELEASE_THRESHOLD = impl.GIL_RELEASE_THRESHOLD Checksum = impl.Checksum -__all__ = ["extend", "value", "Checksum", "implementation"] +__all__ = [ + "extend", + "value", + "Checksum", + "GIL_RELEASE_THRESHOLD", + "implementation", +] diff --git a/packages/google-crc32c/src/google_crc32c/_checksum.py b/packages/google-crc32c/src/google_crc32c/_checksum.py index 0aaf0854ce34..458ff24c4672 100644 --- a/packages/google-crc32c/src/google_crc32c/_checksum.py +++ b/packages/google-crc32c/src/google_crc32c/_checksum.py @@ -21,7 +21,7 @@ class CommonChecksum(object): This class should not be used directly and requires an update implementation. Args: - initial_value (Optional[bytes]): the initial chunk of data from + initial_value (Optional[bytes-like]): the initial chunk of data from which the CRC32C checksum is computed. Defaults to b''. """ @@ -36,7 +36,7 @@ def update(self, data): """Update the checksum with a new chunk of data. Args: - chunk (Optional[bytes]): a chunk of data used to extend + chunk (Optional[bytes-like]): a chunk of data used to extend the CRC32C checksum. """ raise NotImplementedError diff --git a/packages/google-crc32c/src/google_crc32c/_crc32c.c b/packages/google-crc32c/src/google_crc32c/_crc32c.c index b20b94d5542d..b6eee928651f 100644 --- a/packages/google-crc32c/src/google_crc32c/_crc32c.c +++ b/packages/google-crc32c/src/google_crc32c/_crc32c.c @@ -6,35 +6,54 @@ static const Py_ssize_t gil_threshold = 1024 * 1024; static int -_should_release_gil(Py_ssize_t length, PyObject *chunk_obj) +_should_release_gil(const Py_buffer *chunk) { - /* Checks if the chunk is immutable (bytes) to prevent concurrent modification, - * and large enough to benefit from releasing the GIL. */ - return (length >= gil_threshold && PyBytes_Check(chunk_obj)); + /* Release the GIL only for chunks of at least gil_threshold bytes backed + * by an immutable bytes object (a bytes instance or a memoryview of one). */ + PyObject *owner = chunk->obj; + + if (chunk->len < gil_threshold || owner == NULL) { + return 0; + } + if (PyMemoryView_Check(owner)) { + owner = PyMemoryView_GET_BASE(owner); + } + return owner != NULL && PyBytes_Check(owner); } -static PyObject * -_crc32c_extend(PyObject *self, PyObject *args) +static uint32_t +_extend_buffer(uint32_t crc, const Py_buffer *chunk) { - unsigned long crc_input; - uint32_t crc; - const char *chunk; - Py_ssize_t length; PyThreadState *save = NULL; - if (!PyArg_ParseTuple(args, "ky#", &crc_input, &chunk, &length)) - return NULL; - - if (_should_release_gil(length, PyTuple_GET_ITEM(args, 1))) { + if (_should_release_gil(chunk)) { save = PyEval_SaveThread(); } - crc = crc32c_extend((uint32_t)crc_input, (const uint8_t*)chunk, length); + crc = crc32c_extend(crc, (const uint8_t*)chunk->buf, (size_t)chunk->len); if (save) { PyEval_RestoreThread(save); } + return crc; +} + +static PyObject * +_crc32c_extend(PyObject *self, PyObject *args) +{ + unsigned long crc_input; + uint32_t crc; + Py_buffer chunk; + + /* "y*" accepts any C-contiguous object supporting the buffer protocol + * (bytes, bytearray, memoryview, array.array, mmap, ...). */ + if (!PyArg_ParseTuple(args, "ky*", &crc_input, &chunk)) + return NULL; + + crc = _extend_buffer((uint32_t)crc_input, &chunk); + PyBuffer_Release(&chunk); + return PyLong_FromUnsignedLong(crc); } @@ -43,22 +62,14 @@ static PyObject * _crc32c_value(PyObject *self, PyObject *args) { uint32_t crc; - const char *chunk; - Py_ssize_t length; - PyThreadState *save = NULL; + Py_buffer chunk; - if (!PyArg_ParseTuple(args, "y#", &chunk, &length)) + if (!PyArg_ParseTuple(args, "y*", &chunk)) return NULL; - if (_should_release_gil(length, PyTuple_GET_ITEM(args, 0))) { - save = PyEval_SaveThread(); - } - - crc = crc32c_value((const uint8_t*)chunk, length); - - if (save) { - PyEval_RestoreThread(save); - } + /* crc32c_value(data, n) is crc32c_extend(0, data, n). */ + crc = _extend_buffer(0, &chunk); + PyBuffer_Release(&chunk); return PyLong_FromUnsignedLong(crc); } @@ -86,5 +97,13 @@ static struct PyModuleDef crc32cmodule = { PyMODINIT_FUNC PyInit__crc32c(void) { - return PyModule_Create(&crc32cmodule); + PyObject *m = PyModule_Create(&crc32cmodule); + if (m == NULL) { + return NULL; + } + if (PyModule_AddIntConstant(m, "GIL_RELEASE_THRESHOLD", (long)gil_threshold) < 0) { + Py_DECREF(m); + return NULL; + } + return m; } diff --git a/packages/google-crc32c/src/google_crc32c/cext.py b/packages/google-crc32c/src/google_crc32c/cext.py index ec22abec5c1c..95ebe49bf788 100644 --- a/packages/google-crc32c/src/google_crc32c/cext.py +++ b/packages/google-crc32c/src/google_crc32c/cext.py @@ -17,6 +17,7 @@ import google_crc32c.__config__ # noqa: F401 from google_crc32c._checksum import CommonChecksum from google_crc32c._crc32c import ( # type: ignore + GIL_RELEASE_THRESHOLD, extend, value, ) @@ -26,8 +27,10 @@ class Checksum(CommonChecksum): """Hashlib-alike helper for CRC32C operations. Args: - initial_value (Optional[bytes]): the initial chunk of data from - which the CRC32C checksum is computed. Defaults to b''. + initial_value (Optional[bytes-like]): the initial chunk of data from + which the CRC32C checksum is computed. Any C-contiguous object + supporting the buffer protocol (``bytes``, ``bytearray``, + ``memoryview``, ...) is accepted. Defaults to b''. """ __slots__ = ("_crc",) @@ -39,7 +42,10 @@ def update(self, chunk): """Update the checksum with a new chunk of data. Args: - chunk (Optional[bytes]): a chunk of data used to extend + chunk (Optional[bytes-like]): a chunk of data used to extend the CRC32C checksum. """ self._crc = extend(self._crc, chunk) + + +__all__ = ["extend", "value", "Checksum", "GIL_RELEASE_THRESHOLD"] diff --git a/packages/google-crc32c/src/google_crc32c/python.py b/packages/google-crc32c/src/google_crc32c/python.py index 04cedabffe26..1d98ac4885ee 100644 --- a/packages/google-crc32c/src/google_crc32c/python.py +++ b/packages/google-crc32c/src/google_crc32c/python.py @@ -16,14 +16,17 @@ from google_crc32c._checksum import CommonChecksum +GIL_RELEASE_THRESHOLD = 1024 * 1024 + def extend(crc, chunk): """Update an existing CRC checksum with new chunk of data. Args crc (int): An existing CRC check sum. - chunk (Union[bytes, List[int], Tuple[int]]): A new chunk of data. - Intended to be a byte string or similar. + chunk (Union[bytes-like, Iterable[int]]): A new chunk of data. + Bytes-like objects are hashed as raw bytes; an iterable of + ints in ``range(256)`` is also accepted. Returns int: New CRC checksum computed by extending existing CRC @@ -39,8 +42,9 @@ def value(chunk): """Compute a CRC checksum for a chunk of data. Args - chunk (Union[bytes, List[int], Tuple[int]]): A new chunk of data. - Intended to be a byte string or similar. + chunk (Union[bytes-like, Iterable[int]]): A new chunk of data. + Bytes-like objects are hashed as raw bytes; an iterable of + ints in ``range(256)`` is also accepted. Returns int: New CRC checksum computed for ``chunk``. @@ -54,31 +58,38 @@ class Checksum(CommonChecksum): """Hashlib-alike helper for CRC32C operations. Args: - initial_value (Optional[bytes]): the initial chunk of data from + initial_value (Optional[bytes-like]): the initial chunk of data from which the CRC32C checksum is computed. Defaults to b''. """ def __init__(self, initial_value=b""): self._crc = 0 - if initial_value != b"": - self.update(initial_value) + self.update(initial_value) def update(self, data): """Update the checksum with a new chunk of data. Args: - chunk (Optional[bytes]): a chunk of data used to extend + data (Optional[bytes-like]): a chunk of data used to extend the CRC32C checksum. """ - if not isinstance(data, array.array) or data.itemsize != 1: - buffer = array.array("B", data) - else: - buffer = data - self._crc = self._crc ^ 0xFFFFFFFF - for b in buffer: - table_poly = _TABLE[(b ^ self._crc) & 0xFF] - self._crc = table_poly ^ ((self._crc >> 8) & 0xFFFFFFFF) - self._crc = self._crc ^ 0xFFFFFFFF + try: + view = memoryview(data) + except TypeError: + # Not a buffer: an iterable of ints in range(256). + view = memoryview(array.array("B", data)) + with view: + # Hash the raw bytes, like the C extension. + if not view.c_contiguous: + raise BufferError("memoryview: underlying buffer is not C-contiguous") + if not view.nbytes: + return # Nothing to hash; cast() rejects empty N-D views. + with view.cast("B") as buffer: + self._crc = self._crc ^ 0xFFFFFFFF + for b in buffer: + table_poly = _TABLE[(b ^ self._crc) & 0xFF] + self._crc = table_poly ^ ((self._crc >> 8) & 0xFFFFFFFF) + self._crc = self._crc ^ 0xFFFFFFFF # fmt:off diff --git a/packages/google-crc32c/tests/test___init__.py b/packages/google-crc32c/tests/test___init__.py index 8d108ac91657..90bcc6e3cb9e 100644 --- a/packages/google-crc32c/tests/test___init__.py +++ b/packages/google-crc32c/tests/test___init__.py @@ -11,8 +11,14 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import array +import ctypes import functools import itertools +import mmap +import sys +import threading +import time from unittest import mock import pytest # type: ignore @@ -194,6 +200,211 @@ def test_value(chunk, expected): assert google_crc32c.value(bytes(chunk)) == expected +_BYTES_LIKE = [ + pytest.param(bytes, id="bytes"), + pytest.param(bytearray, id="bytearray"), + pytest.param(memoryview, id="memoryview"), + pytest.param( + # A view into the middle of a larger buffer: the slice, not the + # exporter's whole buffer, must be hashed. + lambda data: memoryview(b"\x00" * 3 + data + b"\xff" * 5)[3:-5], + id="memoryview-slice", + ), + pytest.param(lambda data: memoryview(bytearray(data)), id="memoryview-writable"), + pytest.param( + lambda data: memoryview(bytearray(data)).toreadonly(), + id="memoryview-readonly", + ), + pytest.param(lambda data: array.array("B", data), id="array"), + # Multi-byte, 'c' and N-D exporters: their raw bytes are hashed, not items. + pytest.param(lambda data: array.array("H", data), id="array-H"), + pytest.param(lambda data: array.array("I", data), id="array-I"), + pytest.param(lambda data: memoryview(data).cast("I"), id="memoryview-cast-I"), + pytest.param(lambda data: memoryview(data).cast("c"), id="memoryview-cast-c"), + pytest.param( + lambda data: memoryview(data).cast("B", [2, len(data) // 2]), + id="memoryview-2d", + ), + pytest.param( + lambda data: (ctypes.c_uint16 * (len(data) // 2)).from_buffer_copy(data), + id="ctypes-uint16", + ), +] + + +@pytest.mark.parametrize("factory", _BYTES_LIKE) +def test_value_w_bytes_like(_crc32c, factory): + assert _crc32c.value(factory(ISCSI_BYTES)) == ISCSI_CRC + + +@pytest.mark.parametrize("factory", _BYTES_LIKE) +def test_extend_w_bytes_like(_crc32c, factory): + # 8-byte chunks hold a whole number of items for every factory. + chunks = [factory(bytes(chunk)) for chunk in iscsi_chunks(8)] + assert functools.reduce(_crc32c.extend, chunks, 0) == ISCSI_CRC + + +def test_value_w_mmap(_crc32c): + # Closing the map raises BufferError if an export was not released. + with mmap.mmap(-1, ISCSI_LENGTH) as buffer: + buffer.write(ISCSI_BYTES) + assert _crc32c.value(buffer) == ISCSI_CRC + assert _crc32c.extend(0, buffer) == ISCSI_CRC + + +@pytest.mark.parametrize( + "empty", + [ + pytest.param(array.array("I"), id="array-I"), + pytest.param((ctypes.c_uint8 * 0 * 3)(), id="ctypes-2d"), + ], +) +def test_value_w_empty_bytes_like(_crc32c, empty): + assert _crc32c.value(empty) == EMPTY_CRC + assert _crc32c.extend(123, empty) == 123 + + +def test_value_w_iterable_of_ints(): + from google_crc32c import python + + assert python.value(ISCSI_SCSI_READ_10_COMMAND_PDU) == ISCSI_CRC + assert python.value(iter(ISCSI_SCSI_READ_10_COMMAND_PDU)) == ISCSI_CRC + + +def test_value_w_str(_crc32c): + with pytest.raises(TypeError): + _crc32c.value("DEADBEEF") + + +def test_gil_release_threshold(_crc32c): + assert _crc32c.GIL_RELEASE_THRESHOLD == 1024 * 1024 + assert google_crc32c.GIL_RELEASE_THRESHOLD == 1024 * 1024 + + +# Larger than the 1 MiB threshold above which the C extension releases the +# GIL for chunks backed by bytes. CRC from the pure-Python implementation. +LARGE = ISCSI_BYTES * (2 * 1024 * 1024 // ISCSI_LENGTH + 1) +LARGE_CRC = 0x29CF5F4B + + +@pytest.mark.parametrize("factory", _BYTES_LIKE) +def test_value_w_large_bytes_like(_cext, factory): + assert _cext.value(factory(LARGE)) == LARGE_CRC + assert _cext.extend(0, factory(LARGE)) == LARGE_CRC + + +def test_value_w_large_readonly_view_of_bytearray_releases_buffer(_cext): + buffer = bytearray(LARGE) + view = memoryview(buffer).toreadonly() + assert _cext.value(view) == LARGE_CRC + assert _cext.extend(0, view) == LARGE_CRC + view.release() # BufferError if the extension kept its export. + buffer.append(0) + + +@pytest.mark.parametrize( + "factory", + [ + pytest.param(lambda buffer: buffer, id="bytearray"), + pytest.param(lambda buffer: memoryview(buffer).toreadonly(), id="readonly"), + ], +) +def test_value_holds_gil_for_mutable_memory(_cext, factory): + # Read-only views of mutable memory can still be written through their + # owner, so the GIL must stay held while they are hashed. With a 10 s + # switch interval this thread keeps the GIL until value() releases it or + # join() blocks, so the probe sees in_call=True only if value() did. + data = factory(bytearray(64 * 1024 * 1024)) + go = threading.Event() + in_call = False + seen = [] + + def probe(): + go.wait() + seen.append(in_call) + + interval = sys.getswitchinterval() + sys.setswitchinterval(10) + try: + thread = threading.Thread(target=probe, daemon=True) + thread.start() + go.set() + in_call = True + _cext.value(data) + in_call = False + thread.join() + finally: + sys.setswitchinterval(interval) + assert seen == [False] + + +@pytest.mark.parametrize( + "factory", + [ + pytest.param(lambda data: data, id="bytes"), + pytest.param(lambda data: memoryview(data), id="memoryview"), + pytest.param(lambda data: memoryview(data)[1:-1], id="slice"), + ], +) +def test_value_releases_gil_for_immutable_memory(_cext, factory): + # The counterpart of the test above: nothing can write to a bytes object, + # so hashing it (or a view into it) runs with the GIL released and the + # probe thread gets to run while value() is in progress. + data = factory(bytes(2 * _cext.GIL_RELEASE_THRESHOLD)) + go = threading.Event() + in_call = False + seen = [] + + def probe(): + go.wait() + seen.append(in_call) + + interval = sys.getswitchinterval() + sys.setswitchinterval(10) + try: + thread = threading.Thread(target=probe, daemon=True) + thread.start() + in_call = True + go.set() + # On a busy machine one call can finish before the probe is scheduled, + # so hash until it has run. The deadline stays below the switch + # interval: only a released GIL lets the probe in during the loop. + deadline = time.monotonic() + 5 + while not seen and time.monotonic() < deadline: + _cext.value(data) + in_call = False + thread.join() + finally: + sys.setswitchinterval(interval) + assert seen == [True] + + +def _fortran_order(data): + np = pytest.importorskip("numpy") + # Only reached when numpy is installed, which the unit session does not do. + return memoryview( # pragma: NO COVER + np.frombuffer(data, np.uint8).reshape(2, -1, order="F") + ) + + +_NON_C_CONTIGUOUS = [ + pytest.param(lambda data: memoryview(data)[::2], id="strided"), + pytest.param(_fortran_order, id="fortran-order"), +] + + +@pytest.mark.parametrize("factory", _NON_C_CONTIGUOUS) +def test_value_w_non_contiguous_memoryview(_crc32c, factory): + with pytest.raises(BufferError): + _crc32c.value(factory(ISCSI_BYTES)) + + +@pytest.mark.parametrize("factory", _NON_C_CONTIGUOUS) +def test_extend_w_non_contiguous_memoryview(_crc32c, factory): + with pytest.raises(BufferError): + _crc32c.extend(0, factory(ISCSI_BYTES)) + + def pytest_generate_tests(metafunc): if "_crc32c" in metafunc.fixturenames: metafunc.parametrize("_crc32c", ["python", "cext"], indirect=True) @@ -216,6 +427,16 @@ def _crc32c(request): raise ValueError("invalid internal test config") +@pytest.fixture +def _cext(): + try: + from google_crc32c import cext + except ImportError: # pragma: NO COVER + pytest.skip("C extension not compiled") # pragma: NO COVER + + return cext # pragma: NO COVER + + class TestChecksum(object): @staticmethod def test_ctor_defaults(_crc32c): @@ -228,6 +449,23 @@ def test_ctor_explicit(_crc32c): helper = google_crc32c.Checksum(chunk) assert helper._crc == google_crc32c.value(chunk) + @staticmethod + @pytest.mark.parametrize("factory", _BYTES_LIKE) + def test_ctor_w_bytes_like(_crc32c, factory): + helper = _crc32c.Checksum(factory(ISCSI_BYTES)) + assert helper._crc == ISCSI_CRC + + @staticmethod + def test_ctor_w_exporter_whose_ne_raises(_crc32c): + # Like a numpy array, whose ``!=`` compares element-wise and has no + # truth value, so the constructor must not compare initial_value. + class NoNotEqual(bytes): + def __ne__(self, other): + raise TypeError("compared") # pragma: NO COVER + + helper = _crc32c.Checksum(NoNotEqual(ISCSI_BYTES)) + assert helper._crc == ISCSI_CRC + @staticmethod def test_update_array(): import array @@ -246,6 +484,13 @@ def test_update(_crc32c): helper.update(chunk) assert helper._crc == google_crc32c.value(chunk) + @staticmethod + @pytest.mark.parametrize("factory", _BYTES_LIKE) + def test_update_w_bytes_like(_crc32c, factory): + helper = _crc32c.Checksum() + helper.update(factory(ISCSI_BYTES)) + assert helper._crc == ISCSI_CRC + @staticmethod def test_update_w_multiple_chunks(_crc32c): helper = google_crc32c.Checksum()