Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use isinstance in builtin crc32 #2329

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Use isinstance in builtin crc32 #2329

merged 1 commit into from
Aug 8, 2023

Conversation

FlxB2
Copy link
Contributor

@FlxB2 FlxB2 commented Oct 3, 2022

I'm in the process of building a dynamic analysis tool for python, I ran it on this repo and found a potential improvement.

Right now the builtin crc32 module uses type(data) != array.array.
However, using not isinstance(data, array.array) has a number of advantages:

  • array.array subtypes are supported
  • follows PEP-008 recommendations
  • better readability
  • performance improvement

I measured the performance as follows:

$ python3.9 -m timeit -s "from _crc32c import crc_update_faster; import array" -u nsec "crc_update_faster(0, array.array('B', b''))"
500000 loops, best of 5: 576 nsec per loop
$ python3.9 -m timeit -s "from _crc32c import crc_update; import array" -u nsec "crc_update(0, array.array('B', b''))"
500000 loops, best of 5: 622 nsec per loop
$ python3.9 --version
Python 3.9.14

where crc_update_faster uses not isinstance(data, array.array).

You can find a more detailed explanation why isinstance might be better in this blogpost.


This change is Reviewable

@wbarnha wbarnha self-requested a review August 3, 2023 03:23
@wbarnha
Copy link
Contributor

wbarnha commented Aug 3, 2023

Thanks for profiling this! Using isinstance for checks is always a good choice.

@wbarnha wbarnha merged commit d920108 into dpkp:master Aug 8, 2023
bradenneal1 pushed a commit to bradenneal1/kafka-python that referenced this pull request May 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants