Skip to content

gh-125442: Use _PyLong_GetOne() and _PyLong_GetZero() in Objects #125440

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

Closed
wants to merge 4 commits into from
Closed

gh-125442: Use _PyLong_GetOne() and _PyLong_GetZero() in Objects #125440

wants to merge 4 commits into from

Conversation

hikariyo
Copy link

@hikariyo hikariyo commented Oct 14, 2024

Simply reduces a null check as _PyLong_GetOne() cannot fail.

@ghost
Copy link

ghost commented Oct 14, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@skirpichev
Copy link
Member

There are other places, that might be changed, especially in longobject.c:

$ git grep 'PyLong_From[IL].*([10]' Objects/|wc -l
22

I suggest you first create an issue.

@hikariyo hikariyo changed the title Use _PyLong_GetOne() in long_pow gh-125442: Use _PyLong_GetOne() in longobject.c Oct 14, 2024
@hikariyo hikariyo requested a review from ethanfurman as a code owner October 14, 2024 11:00
@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@hikariyo hikariyo changed the title gh-125442: Use _PyLong_GetOne() in longobject.c gh-125442: Use _PyLong_GetOne() in Objects Oct 14, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@hikariyo hikariyo changed the title gh-125442: Use _PyLong_GetOne() in Objects gh-125442: Use _PyLong_GetOne() and _PyLong_GetZero() in Objects Oct 14, 2024
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Do not merge until there are solid evidences of the speed up.

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@skirpichev
Copy link
Member

@serhiy-storchaka, why do you suspect speed regressions at all? PyLong_FromLong() will run more complex versions of _PyLong_GetZero/One anyway.

PyLong_FromLong(long ival)
{
PyLongObject *v;
unsigned long abs_ival, t;
int ndigits;
/* Handle small and medium cases. */
if (IS_SMALL_INT(ival)) {
return get_small_int((sdigit)ival);

static PyObject *
get_small_int(sdigit ival)
{
assert(IS_SMALL_INT(ival));
return (PyObject *)&_PyLong_SMALL_INTS[_PY_NSMALLNEGINTS + ival];
}

// Return a reference to the immortal zero singleton.
// The function cannot return NULL.
static inline PyObject* _PyLong_GetZero(void)
{ return (PyObject *)&_PyLong_SMALL_INTS[_PY_NSMALLNEGINTS]; }
// Return a reference to the immortal one singleton.
// The function cannot return NULL.
static inline PyObject* _PyLong_GetOne(void)
{ return (PyObject *)&_PyLong_SMALL_INTS[_PY_NSMALLNEGINTS+1]; }

@hikariyo
Copy link
Author

hikariyo commented Oct 14, 2024

@serhiy-storchaka I'm inspired by #125044 and I found that there are other places using PyLong_FromLong for 0 or 1.

At least the 2 null checks below should be removed. They're unreachable.

cpython/Objects/floatobject.c

Lines 1564 to 1566 in 5dac0dc

denominator = PyLong_FromLong(1);
if (denominator == NULL)
goto error;

cpython/Objects/longobject.c

Lines 5006 to 5008 in 5dac0dc

z = (PyLongObject *)PyLong_FromLong(1L);
if (z == NULL)
goto Error;

@skirpichev
Copy link
Member

@hikariyo, all similar checks are unreachable now. But that's might be viewed as internal detail of the PyLong_FromLong() implementation.

@serhiy-storchaka
Copy link
Member

I suspect no difference at all. This is a pure cosmetic change. On other hand, it replaces public functions calls with private functions calls. So, this is a cosmetic change that makes the code more ugly.

@hikariyo
Copy link
Author

@hikariyo, all similar checks are unreachable now. But that's might be viewed as internal detail of te PyLong_FromLong() implementation.

However _PyLong_GetZero/One must return non-null values, so using them can avoid this problem.

@hikariyo
Copy link
Author

I suspect no difference at all. This is a pure cosmetic change. On other hand, it replaces public functions calls with private functions calls. So, this is a cosmetic change that makes the code more ugly.

But _PyLong_GetZero/One do widely used in current code. Shouldn't we unify the usages?

@serhiy-storchaka
Copy link
Member

They were introduced (first as simple global variables) for the cases when you need a borrowed reference to integers 0 or 1. PyLong_FromLong() continued to be used in cases when you need a new reference.

Closing this PR as pure cosmetic change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants