[QUESTION] Potential dependency on crc32c package

[rtobar]

Hi,

I'm not sure how much inter-communication there is between this project and kafka-python, so I'm opening a separate ticket here to discuss an idea I discussed over there that applies to this project as well. This is more of a discussion really, but I couldn't see the "Discussions" section enabled on this project, so I'm filing it as a "question" issue instead.

In dpkp/kafka-python#2380 I mention how during some profiling we observed some bottlenecks around crc32c calculation because, for some reason, the python fallback routines were being used. After doing a deep dive, I found that both this project and kafka-python have python fallbacks, while at the same time catering for faster versions of the checksums: this project ships with its own C/Cython-based extension, while kafka-python optionally uses the crc32c package.

My question there, and now here, is: would you consider adding a hard dependency on crc32c to avoid having python fallbacks in the first place, while also not having to maintain a C/Cython-based module for this purpose?

Please see the above thread for a longer discussion, and thanks in advance for any consideration given to this idea.

[vmaurin]

For me I would take an approach of logging, like I was suggesting here #896

When there is a fallback to a pure python implementation, it could be nice to log something like "Lib xxx not found, fallback to a python implementation of Y, that will result in a performance drop".

So the additional "C" dependencies could remain optional, but it is not using fallback python silently

[vmaurin]

So basically, what I am suggesting is to review the import block to add a logging

if NO_EXTENSIONS:
    calc_crc32c = calc_crc32c_py
    decode_varint = decode_varint_py
    size_of_varint = size_of_varint_py
    encode_varint = encode_varint_py
else:
    try:
        from ._crecords import (  # noqa
            decode_varint_cython, crc32c_cython, encode_varint_cython,
            size_of_varint_cython
        )
        decode_varint = decode_varint_cython
        encode_varint = encode_varint_cython
        size_of_varint = size_of_varint_cython
        calc_crc32c = crc32c_cython
    except ImportError as err:  # pragma: no cover
        logger.warning("Failed to load the extension, performance will be impacted", exc_info=err)
        calc_crc32c = calc_crc32c_py
        decode_varint = decode_varint_py
        size_of_varint = size_of_varint_py
        encode_varint = encode_varint_py

[vmaurin]

@rtobar Do you have an example of such a fallback happening in aiokafka ? I am also experiencing lower performance as expected for a stream application, but I discarded the fallback lead as it seems everything in aiokafka is embedded and only depend on libc that should be in my docker container running the application

[rtobar]

This is with python 3.10 and poetry 1.5.1:

mkdir tmp
cd tmp
poetry new .
poetry add aiokafka==0.7.2 # we had this version pinned, which doesn't have binary wheel for 3.10, something we realised only as part of this profiling exercise
poetry shell
$> ls $VIRTUAL_ENV/lib/python3.11/site-packages/aiokafka/record/_crecords/
consts.pxi  crc32c.c  crc32c.h  cutil.c  cutil.pxd  cutil.pyx  default_records.c  default_records.pxd  default_records.pyx  hton.pxd  __init__.py  legacy_records.c  legacy_records.pxd  legacy_records.pyx  memory_records.c  memory_records.pyx

So the sources are there, but the installation still didn't result in the cython extension being compiled into a dynlib module.

[rtobar]

BTW, pip install aiokafka==0.7.2 also shows the actual error, but maybe this is a known, old problem, so I'm just posting the error for reference.

My original point though is that this kind of situations could be avoided altogether by depending on a third-party package that did this for you, while also having a few more benefits (has ARMv8 acceleration, software C impl works on big-endian CPUs).

  creating build/temp.linux-x86_64-cpython-311/aiokafka/record/_crecords
  x86_64-linux-gnu-gcc -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -fPIC -I/home/rtobar/.cache/pypoetry/virtualenvs/tmp-lScooxj2-py3.11/include -I/usr/include/python3.11 -c aiokafka/record/_crecords/legacy_records.c -o build/temp.linux-x86_64-cpython-311/aiokafka/record/_crecords/legacy_records.o -O2 -Wall -Wsign-compare -Wconversion
  aiokafka/record/_crecords/legacy_records.c:218:12: fatal error: longintrepr.h: No such file or directory
    218 |   #include "longintrepr.h"
        |            ^~~~~~~~~~~~~~~

[rtobar]

Two other points I'd like to add to this discussion:

• Falling back to the python implementation means that, depending on the size of the data being checksumed, the time spent in validation can be high enough for PYTHONASYNCIODEBUG=1 to highlight that the consumer.getone() coroutine takes a long time to yield back control to the IO loop, hence blocking other coroutines. We hit this issue in our tests, where a completely unrelated coroutine was failing because it wasn't scheduled within a reasonable time window. This happens because

  aiokafka/aiokafka/consumer/fetcher.py

  Line 212 in abd39be

  if self._check_crcs and not next_batch.validate_crc():

  is called in the context of a coroutine. In our experience, and depending on system load and other factors, this can lead to >2s spent in this coroutine alone, which doesn't happen when using the Cython extension (or the crc32c package, if that were still to be considered as an option).
• About using crc32c: If used, you'd get some benefits, like accelerated ARMv8 support and big-endian CPU support for the software version, plus the added benefit of not having to maintain that corner of the code. But OTOH I understand the hesitation to add a strong dependency on it. I just wanted to make those extra additional benefits more explicit.

[rtobar]

Another update: for those who haven't measure it, in dpkp/kafka-python#2380 (comment) I show the effects of falling back to the python implementation. In our synthetic benchmarks it was >25x in some cases.