Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

optimise encoding and decoding #76

Merged
merged 3 commits into from
Jul 25, 2024
Merged

optimise encoding and decoding #76

merged 3 commits into from
Jul 25, 2024

Conversation

GiacomoPope
Copy link
Owner

bit operations are slow and big integers are fast because python. About 1.5 - 2x performance increase and simpler functions.

@GiacomoPope
Copy link
Owner Author

@tomato42 this breaks because Python 3.9 doesnt support bit_count() -- I can fix this, but I'm not actually super interested in it working for 3.9, is the reason you want to go back to 3.9 because its not end of life?

The 3.9 version of this is bin(x).count("1") which is a pretty ugly hack.

@GiacomoPope
Copy link
Owner Author

def bit_count(x):
    """
    Count the number of bits in x

    Method to support old python as `x.bit_count()`
    was released in Python 3.10 and we currently
    support Python 3.9
    """
    try:
        return x.bit_count()
    except AttributeError:
        return bin(x).count("1")

Begrudgingly wrote something stupid for python 3.9

@tomato42
Copy link
Collaborator

@tomato42 this breaks because Python 3.9 doesnt support bit_count() -- I can fix this, but I'm not actually super interested in it working for 3.9, is the reason you want to go back to 3.9 because its not end of life?

The 3.9 version of this is bin(x).count("1") which is a pretty ugly hack.

I need 3.9 because that's the version that will be supported for the lifetime of RHEL-9

@tomato42
Copy link
Collaborator

Begrudgingly wrote something stupid for python 3.9

the downside of that is that it will be very inefficient in 3.9, and we know the problem is with 3.9, so it's better to define a different method conditionally, based on detected python version

@GiacomoPope
Copy link
Owner Author

Sure but if we do a conditional check then it means every python version gets the performance hit by having to do a check before the function. I'd rather just make python 3.9 slower (this string counting is already 6x slower than the 3.10 version so the extra try/except is relatively small.

@GiacomoPope
Copy link
Owner Author

Also the CI says failure because of the coverage dropping but I thought it was supposed to be taking into account all the tests from 3.9-3.12 -- so shouldn't both branches of this new function be considered?

@tomato42
Copy link
Collaborator

tomato42 commented Jul 24, 2024

Sure but if we do a conditional check then it means every python version gets the performance hit by having to do a check before the function. I'd rather just make python 3.9 slower (this string counting is already 6x slower than the 3.10 version so the extra try/except is relatively small.

No, that check is executed then only once, at the time the module is loaded, not every time the function is executed

@tomato42
Copy link
Collaborator

Also the CI says failure because of the coverage dropping but I thought it was supposed to be taking into account all the tests from 3.9-3.12 -- so shouldn't both branches of this new function be considered?

that's because you reduced the amount of code, so it increased the fraction of code that is not covered

@GiacomoPope GiacomoPope merged commit ab4f388 into main Jul 25, 2024
9 of 14 checks passed
@GiacomoPope GiacomoPope deleted the optimise_encode_decode branch July 25, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants