Skip to content

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

Closed
hikariyo wants to merge 4 commits into
python:mainfrom
hikariyo:main
Closed

gh-125442: Use _PyLong_GetOne() and _PyLong_GetZero() in Objects#125440
hikariyo wants to merge 4 commits into
python:mainfrom
hikariyo:main

Conversation

@hikariyo

@hikariyo hikariyo commented Oct 14, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown

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

@ghost

ghost commented Oct 14, 2024 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
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
@bedevere-app bedevere-app Bot mentioned this pull request Oct 14, 2024
Closed
@hikariyo
hikariyo requested a review from ethanfurman as a code owner October 14, 2024 11:00
@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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 commented Oct 14, 2024
View reviewed changes
Comment thread Objects/iterobject.c
@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
serhiy-storchaka
serhiy-storchaka requested changes Oct 14, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
Member

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

cpython/Objects/longobject.c

Lines 309 to 317 in 5dac0dc

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);

cpython/Objects/longobject.c

Lines 56 to 61 in 5dac0dc

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

cpython/Include/internal/pycore_long.h

Lines 66 to 74 in 5dac0dc

// 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

hikariyo commented Oct 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

@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

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
Member

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

@serhiy-storchaka

serhiy-storchaka commented Oct 14, 2024

Copy link
Copy Markdown
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

hikariyo commented Oct 14, 2024

Copy link
Copy Markdown
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

hikariyo commented Oct 14, 2024

Copy link
Copy Markdown
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

serhiy-storchaka commented Oct 14, 2024

Copy link
Copy Markdown
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

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka requested changes
@ethanfurman ethanfurman Awaiting requested review from ethanfurman

Assignees

No one assigned

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hikariyo @skirpichev @serhiy-storchaka @picnixz