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

Remove unnecessary calls to PyNumber_Index, PyLong_Checks #3242

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented Nov 25, 2024

This simplifies code and improves performance. It makes rect[x] ~7% faster and rect[x] = y ~9% faster. Although these functions are really cheap, and therefore I would be surprised if there was any measurable macro impact from this change. You can run them 10 million times in less than a second. I didn't test performance impact in pixelarray.

And in case anyone is curious rect.y is ~8% faster than rect[1] (after this PR). So for those micro-optimizers out there, that's the strategy to use.

PyNumber_AsSsize_t doesn't need the value to be converted to an "index int" first.

If you look at the CPython source, in Objects/abstract.c:

Py_ssize_t
PyNumber_AsSsize_t(PyObject *item, PyObject *err)
{
    Py_ssize_t result;
    PyObject *runerr;
    PyObject *value = _PyNumber_Index(item);
    if (value == NULL)
        return -1;
    ...

The first thing it does is use PyNumber_Index anyway!

Plus it is documented as "Returns o converted to a Py_ssize_t value if o can be interpreted as an integer". Having an __index__ method counts as being interpretable as an integer.

PyIndex_Check(op) || PyLong_Check(op), the PyLong_Check is unnecessary.

Python ints already pass PyIndex_Check, as you can tell because Python ints work in the Rect functions.

@Starbuck5 Starbuck5 requested a review from a team as a code owner November 25, 2024 07:10
@Starbuck5 Starbuck5 added Code quality/robustness Code quality and resilience to changes rect pygame.rect PixelArray pygame.PixelArray labels Nov 25, 2024
Copy link
Contributor

@yunline yunline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Testing code:

import pygame
pygame.init()

class IndexLike:
    def __index__(self):
        raise RuntimeError("Nope")

r = pygame.Rect(0,0,1,1)

r[1<<256] # IndexError: Invalid rect Index
r[IndexLike()] # RuntimeError: Nope

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 🎈

@ankith26 ankith26 added this to the 2.5.3 milestone Nov 27, 2024
@ankith26 ankith26 merged commit 2913d6f into pygame-community:main Nov 27, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes PixelArray pygame.PixelArray rect pygame.rect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants